-
Notifications
You must be signed in to change notification settings - Fork 33
Add flip_conjugation method and user info if it would reduce the uvw discrepancy
#1634
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
Conversation
|
It turns out this is causing a lot of test errors. Digging in, there are flips happening when the improvement to the uvw matching to the expected value is quite marginal e.g. 44m max diff instead of 50m. This is giving me bad feelings because it feels quite stochastic -- e.g whether or not it wants to flip can depend on which set of baselines is read in on a partial read. I think we need a different approach here. Options I can think of:
Thoughts? @jpober @mkolopanis @adeliegorce |
|
Hi! Thank you for having a look at this. I do agree point #3 implies a hardcoded upper limit which can be sub-optimal. Personally I think option #1 is best - or a variation: the flip is an additional input to the |
|
Ah, yeah, I was worried something like that might happen. I think I agree that presenting the user with an option to flip conjugation is good, perhaps also editing the warning about uvws not matching in the ms reader to suggest that sometimes a conjugation flip can be the issue. |
Add info to uvw check warning message to inform users when a conjugation flip might be helpful.
flip_conjugation method and user info if it would reduce the uvw discrepancy
|
Ok, I took a stab at it. It now has the old behavior, but adds some info to the warning message to inform users when a conjugation flip might help and points to the method that can do the flip. edit: I realized that I didn't actually add it as an option to the Let me know what you think @adeliegorce @jpober |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1634 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 67 67
Lines 22627 22634 +7
=======================================
+ Hits 22613 22620 +7
Misses 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for solving my issue! |
Description
Pulled the conjugation flip code out as a user-callable method. Added info to the uvw check warning message to inform a user when a conjugation flip might reduce uvw discrepancies.
Motivation and Context
fixes #1632
Types of changes
Checklist:
New feature checklist: