Skip to content

Add back REF/ALT to csv output in addition to resolved GT fields #83

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

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

nalinigans
Copy link
Member

No description provided.

@nalinigans nalinigans requested a review from mlathara March 13, 2025 18:03
// Process REF, ALT and GT first.
process_str_field("REF", calls, dims, sizes);
process_str_field("ALT", calls, dims, sizes);
process_str_field("GT", calls, dims, sizes);
Copy link
Member

Choose a reason for hiding this comment

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

Naive question: can you tell me why we process REF, ALT, GT first instead of in the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

They could be in any order based on how GenomicsDB processes them. We could let the loop handle it, but sometimes, the order is ALT,REF,..,GT. I just thought it nicer to have REF before ALT, guess it does not matter for GT as much.. But, I can process them in the loop if that is OK too. Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, can't decide. My initial concern was that this bit of code gets invoked for each variant so trying to minimize any extraneous code. But that's probably micro-optimization...thoughts?

Copy link
Member Author

@nalinigans nalinigans Mar 13, 2025

Choose a reason for hiding this comment

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

This bit of code is not invoked per variant call, it happens only once per query - see. The calls themselves are already populated as m_string_fields in the dictionary backed by std::map<std::string, std::vector<PyObject *>> m_string_fields as part of the code here. That said, I could probably order them at initialization.

Copy link
Member

Choose a reason for hiding this comment

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

Ah - thanks for that clarification, I misread this as being part of process somehow. I think this is good to merge the way it is if you want.

@nalinigans nalinigans merged commit 474664e into develop Mar 13, 2025
8 checks passed
@nalinigans nalinigans deleted the ng_add_ref_alt branch March 13, 2025 21:25
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