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

break up main annotation function into small pieces #405

Merged
merged 33 commits into from
Apr 17, 2024

Conversation

macklin-10x
Copy link
Contributor

Mechanistic refactoring of the annotation code from a single straight-shot function into a collection of smaller individual pieces. The only non-trivial changes to code were untangling the use of std::mem::swap left over from when the internal annotation types were tuples instead of structs.

@macklin-10x macklin-10x force-pushed the macklin/annotation-internal-types branch from 88b4193 to 8fb4a75 Compare March 22, 2024 18:53
@macklin-10x macklin-10x force-pushed the macklin/break-up-annotation branch from 8bf9191 to 760494f Compare March 22, 2024 18:54
@macklin-10x macklin-10x force-pushed the macklin/annotation-internal-types branch from 8fb4a75 to 276a5f0 Compare March 29, 2024 04:33
@macklin-10x macklin-10x force-pushed the macklin/break-up-annotation branch from 760494f to f048159 Compare March 29, 2024 04:38
Base automatically changed from macklin/annotation-internal-types to main April 2, 2024 04:18
@macklin-10x macklin-10x force-pushed the macklin/break-up-annotation branch from f048159 to 16096b7 Compare April 2, 2024 04:22
Copy link
Contributor

@sreenathkrishnan sreenathkrishnan left a comment

Choose a reason for hiding this comment

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

Really like the small functions with well chosen names

)
};
key(a).cmp(&key(b))
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! A lot cleaner now. This mem swap makes the changes confusing, but looks like we have it right

Comment on lines -2521 to -2549
let mut to_delete: Vec<bool> = vec![false; annx.len()];
let (mut u, mut v) = (Vec::<String>::new(), Vec::<String>::new());
for a in &annx {
let t = a.ref_id as usize;
if !rheaders[t].contains("segment") {
let name = rheaders[t].after("|").between("|", "|");
if rheaders[t].contains("UTR") {
u.push(name.to_string());
}
if rheaders[t].contains("V-REGION") {
v.push(name.to_string());
}
}
}
v.sort();
for item in &u {
if !bin_member(&v, item) {
for j in 0..annx.len() {
let t = annx[j].ref_id as usize;
if !rheaders[t].contains("segment") {
let name = rheaders[t].after("|").between("|", "|");
if rheaders[t].contains("UTR") && item == name {
to_delete[j] = true;
}
}
}
}
}
erase_if(&mut annx, &to_delete);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to get rid of this large duplicated block

@macklin-10x macklin-10x merged commit 548f871 into main Apr 17, 2024
2 checks passed
@macklin-10x macklin-10x deleted the macklin/break-up-annotation branch April 17, 2024 17:35
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