Add nextstrain-open build - #332
Conversation
| vaccines: "config/{lineage}/vaccine.json" | ||
| enable_glycosylation: true | ||
| enable_lbi: true | ||
| enable_embeddings: true |
There was a problem hiding this comment.
Test run in GH Action errored on embed_with_tsne
Traceback (most recent call last):
File "/usr/local/bin/pathogen-embed", line 8, in <module>
sys.exit(run_embed())
^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pathogen_embed/__main__.py", line 109, in run_embed
return embed(args)
^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pathogen_embed/pathogen_embed.py", line 565, in embed
embedding = embedder.fit_transform(distance_matrix)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/sklearn/utils/_set_output.py", line 295, in wrapped
data_to_wrap = f(self, X, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/sklearn/base.py", line 1474, in wrapper
return fit_method(estimator, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/sklearn/manifold/_t_sne.py", line 1135, in fit_transform
self._check_params_vs_input(X)
File "/usr/local/lib/python3.11/site-packages/sklearn/manifold/_t_sne.py", line 846, in _check_params_vs_input
raise ValueError("perplexity must be less than n_samples")
ValueError: perplexity must be less than n_samples
There was a problem hiding this comment.
pathogen-embed should handle that error more gracefully, but the default perplexity is 30. Do you expect to have fewer than 30 samples in the open build? Maybe for a 6-month build there isn't that much more data?
The workaround is to set the perplexity to a smaller value. This is a global setting right now, so it would change embeddings for all the builds. Probably the better short-term fix is to disable embeddings like you had before.
There was a problem hiding this comment.
Do you expect to have fewer than 30 samples in the open build? Maybe for a 6-month build there isn't that much more data?
Hmm, there should be more than 30 samples in the 6m build, but yeah I'll turn off embeddings for now.
Turn off for now since it errors for 6m builds See <#332 (comment)>
66c21fc to
ee0e6b1
Compare
Turn off for now since it errors for 6m builds See <#332 (comment)>
bbf9c0e to
411c533
Compare
huddlej
left a comment
huddlej
left a comment
There was a problem hiding this comment.
This worked well for me out of the box, @joverlee521. I made my own build covering all time for H3N2 and H1N1pdm which forced me to play around with the settings more. Everything worked as expected.
I had a question/suggestions about how we handle Auspice config JSON overrides, but I know that's a sticky subject better had with the rest of the team.
The main blocker I could imagine to releasing open builds based on this is that not all of the vaccine strains from our original builds have matching records in the open data. This is something we could look into outside of this PR, though.
| auspice_config = "data/{lineage}/{segment}/auspice_config.json", | ||
| shell: | ||
| """ | ||
| jq '.data_provenance = [{{"name": "GenSpectrum", "url": "https://loculus.genspectrum.org"}}]' \ |
There was a problem hiding this comment.
Instead of special-casing the provenance of these builds, what if we either:
- add a rule to the main workflow that always adds a provenance based on values defined in the build config (e.g.,
data_provenance: 'gisaid'), so the provenance becomes an explicit configuration parameter instead of implicitly defaulting to one provenance
or
- add an argument to
augur export v2that allows the user/workflow to specify the data provenance as an argument that overrides the config JSON instead of unioning with the config (--data-provenance '[{{"name": "GenSpectrum", "url": "https://loculus.genspectrum.org"}}]') and make the export rule in this workflow pull the provenance from the build config
or
- make copies of the configs for the open data like
config/{lineage}/{segment}/auspice_config_{provenance}.json
I get that this last option is what you're trying to avoid with this custom rule.
There was a problem hiding this comment.
Ah thanks for flagging! This was a quick workaround for the example-open build, so I should definitely revisit.
Yes, definitely trying to avoid (3), so I think (1) is the most appealing.
This also reminds me that we should probably rename the GISAID builds if we are going to move to seasonal-flu/gisaid/... and seasonal-flu/open/... URLs.
There was a problem hiding this comment.
After today's in person discussion, will pivot to layering auspice configs for the separate data provenances. I'll update the PR to define maintainers and data_provenance in separate JSONs that can be layered on top the base segment auspice config.
There was a problem hiding this comment.
I'll update the PR to define maintainers and data_provenance in separate JSONs that can be layered on top the base segment auspice config.
This is now done with aa49e1b...f891b20
| subclades: "config/{lineage}/{{segment}}/subclades.tsv" | ||
| # Created in the workflow with custom rule `overwrite_auspice_config` | ||
| auspice_config: "data/{lineage}/{{segment}}/auspice_config.json" | ||
| vaccines: "config/{lineage}/vaccine.json" |
There was a problem hiding this comment.
Note to self to check that the vaccine strains actually appear in the open data with the same names.
There was a problem hiding this comment.
I found that several H3N2 vaccine strains didn't have exact matches among the strain names in the open metadata (e.g., none of the Swiss strains post-2012, the recent Croatia strain, etc.). We will need to look for corresponding records in GenBank for all of these missing strains across all three subtypes.
| include: "'config/{lineage}/reference_strains.txt'" | ||
| exclude: "'config/{lineage}/outliers.txt'" |
There was a problem hiding this comment.
We may need to define new reference and outlier files for open data or make new entries in the existing files.
There was a problem hiding this comment.
I ended up adding new entries to the existing files during my testing.
Prompted by PR feedback - <#332 (comment)> - <#332 (comment)>
Based on existing example build, with changes to remove the custom rule and switching the `submission_date_field`.
Add subclades to the build config (since subclade is the default coloring in the Auspice config) and subsample the input data to a smaller subset for faster runtime.
Prevents most sequences from being dropped by augur refine's clock rate filter by increasing the threshold from 4 to 10.
We will be using the same description and custom rule for the nextstrain-open builds.
Using the existing public builds as for creating the open builds. The following commit will make changes to fit the open builds.
Edit nextstrain-open config to work with the open data. - overwrite Auspice config - simplify build params since we don't use titer data - remove passage filters since we don't have that metadata
Turn off for now since it errors for 6m builds See <#332 (comment)>
Prompted by PR feedback - <#332 (comment)> - <#332 (comment)>
Doing this in preparation for deleting all of the lineage level auspice configs. We will only use the segment level auspice configs going forward.
Point all config files to use the segment level auspice configs so that we no longer have the extra lineage level auspice configs. Doing this clean up in preparation for splitting out the shared parts of the auspice configs into composable parts.
Create a shared Auspice config for the GISAID data provenance and the maintainers. This allows data provenance and maintainers to be only defined in a central JSON that can be shared with all of the other lineage/segment Auspice configs. Data provenance and maintainers are in separate JSONs because we use the maintainers for the open builds as well.
Removes the need to use an override rule to change the data provenance in the shared configs.
7f46b31 to
776053f
Compare
2 participants
Description of proposed changes
Add example-open and nextstrain-open configs to use the open data.
Related issue(s)
Part of #330
Checklist