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

Depreciate pangolearn #519

Merged
merged 2 commits into from May 9, 2023
Merged

Depreciate pangolearn #519

merged 2 commits into from May 9, 2023

Conversation

rmcolq
Copy link
Contributor

@rmcolq rmcolq commented May 8, 2023

This seemed simpler than I was expecting given the conversations I had with Áine (not tagged deliberately). The intention here is:

  1. If pangolearn or fast mode specified, prints a warning and continues with usher mode instead
  2. If a datadir provided and pangolearn specified, allows pangolearn mode (for backwards compatibility) NB automatically assumes this is an old data dir so versioning will go backwards
  3. The next data release will remove the randomforest files from pangolin-data so that only the up to date models are there.

This all seemed to work when I tested it locally, but would be great to have another set of eyes on it.

@rmcolq rmcolq requested a review from AngieHinrichs May 8, 2023 19:51
Copy link
Member

@AngieHinrichs AngieHinrichs left a comment

Choose a reason for hiding this comment

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

Thanks so much @rmcolq, this looks great! I pulled down the changes and did some testing too, and it all behaved as expected.

I pushed one little change to remove a debugging print statement.

As far as I'm concerned this is fine to merge into the master branch any time. Before we tag the next (pre)release on the repo, I want to make one more little change for v4.3: I want to remove my max_count/max_lineage logic in usher_parsing in report_collation.py that overrides usher's selected lineage with the lineage that gets the highest number of equally parsimonious placements. That's been a bad idea at least since Omicron, maybe earlier. So after this is merged, I'll make that little tweak.

[Looks like I also forgot to update pangolin/data/data_compatibility.csv to include v1.19. And going forward (for pangolin-data v1.20 & beyond) the minimum version in data_compatibility.csv should probably be 4.3 instead of 4.]

@rmcolq rmcolq merged commit ef73f62 into master May 9, 2023
4 checks passed
@rmcolq rmcolq deleted the remove_pangolearn branch May 9, 2023 21:18
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.

None yet

2 participants