Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sequence cache #26

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Add sequence cache #26

merged 2 commits into from
Aug 29, 2024

Conversation

dkhofer
Copy link
Collaborator

@dkhofer dkhofer commented Aug 28, 2024

No description provided.

@dkhofer dkhofer requested a review from Chris7 August 28, 2024 20:57
Copy link
Collaborator

@Chris7 Chris7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Some suggestions on a way to avoid an extra db lookup. I think the lookup use of String will lead to a refactor down the line using this method if we're passing a sequence we want ot reuse from the caller.

pub fn lookup(
sequence_cache: &mut SequenceCache<'a>,
sequence_type: &'a str,
sequence: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this will result in ownership issues where String is moved here but not returned out. &String can resolve that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually fighting the compiler about this yesterday. I couldn't get it to work with &str. I tried with &String just now and got the same problem, which is that the sequence cache exists outside the for loop that's processing the VCF records. The sequence that's being cached gets created inside that for loop, so any reference to it is transient (could go away from one iteration of the for loop to the next), so the sequence reference can't get added to the cache.

src/main.rs Outdated
.sequence_type("DNA")
.sequence(&sequence)
.save(sequence_cache.conn);
let new_sequence =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can actually do this w/o the additional lookup by using the build part of the builder to get the sequence object. So you would have the builder object as a var, call save to get the hash, and build to get the sequence object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done!

@dkhofer dkhofer merged commit 61b1ec0 into main Aug 29, 2024
1 check passed
@Chris7 Chris7 deleted the cache-sequences branch September 24, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants