fix: upper case root sequence for ancestral reconstruction #1323
Conversation
|
Thanks! Seems like a strictly beneficial change, better solutions can be added later. CI failing is because of monkeypox restructure, I will take mpox out of augur CI |
|
Merging this then CI should work again: #1324 |
Makes it possible to reproduce the error and check we actually fix it and don't regress
Makes it possible to reproduce the error and check we actually fix it and don't regress
|
Added a test that failed before the fix so that alternative fixes are easy to test and we don't regress in the future. See: https://github.com/nextstrain/augur/actions/runs/6315326703/job/17147616573#step:9:20 for what the bug looks like |
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
Makes it possible to reproduce the error and check we actually fix it and don't regress
There was a problem hiding this comment.
I don't have enough background to understand why the change is needed. Is it because, for the purpose of ancestral reconstruction, "soft-masking" in the root sequence is irrelevant and should be ignored?
There was a problem hiding this comment.
I didn't know about soft vs hard masking. Indeed, we don't ever treat upper and lower case nucleotides differently anywhere in augur - at least not as far as I'm aware.
There was a problem hiding this comment.
I think you're right - at least for augur index, sequences are lowercased.
There was a problem hiding this comment.
The current commits on this branch are hard for me to follow. Suggested alternative, by rebasing onto latest master and switching the order around: 5aba7ec...07fa7b5
There was a problem hiding this comment.
Just look at the diff vs master - I don't think commit order matters here, it's just a few line changes in general anyways
There was a problem hiding this comment.
Yeah, this is nitpicky and non-blocking! Just want to elaborate on my thoughts here.
My opinion is that "diff vs master" is only applicable if doing a squash merge, where the commits from the PR branch are irrelevant. Without the squash merge, there's unnecessarily verbose commit history which describes the ad-hoc iterative process while working on the PR, rather than the changes that the PR is actually trying to make.
To draw it out, merging as-is will lead to history that looks like this:
* 3c1b5e38 (HEAD -> master) Merge branch 'fix/root-upper-case'
|\
| * f65b47c7 (origin/fix/root-upper-case, fix/root-upper-case) Add changelog entry
| * 7d97e078 Merge branch 'failing-test-for-1323' into fix/root-upper-case
| |\
| | * 5d8bc276 Add failing test for #1323
| * | 1c78c885 Merge branch 'disable-monkeypox-ci' into fix/root-upper-case
| |\|
| | * d5e5dc32 Disable monkeypox CI until pathogen-repo-ci supports custom build directories See issue https://github.com/nextstrain/.github/issues/63
| * | f9c3e198 fix: upper case root sequence for ancestral reconstruction
| |/
* | 9839199f (origin/master, origin/HEAD) Remove outdated note "we have just updated augur to v6"
My suggested alternative:
* 9003db0b (HEAD -> master) Merge branch 'pr-1323-rebased'
|\
| * 07fa7b5a (pr-1323-rebased) Add changelog entry
| * 5ff71be0 fix: upper case root sequence for ancestral reconstruction
| * 57d0123b Add failing test for #1323
* | 9839199f (origin/master, origin/HEAD) Remove outdated note "we have just updated augur to v6"
Given that this PR's scope is small, I think a squash merge is also fine:
* 12324ead (HEAD -> master) fix: upper case root sequence for ancestral reconstruction (#1323)
* 9839199f (origin/master, origin/HEAD) Remove outdated note "we have just updated augur to v6"
There was a problem hiding this comment.
Squash merge seems like the right thing to do here!
I ran into this issue when the root sequence was provided as lower case. All lower-to-upper case changes were attached to the root. This should fix it, but there might be better solutions.