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

[h5n1-cattle-outbreak] show timetree for genome build #98

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

jameshadfield
Copy link
Member

Resolves #94

Tested by building all h5n1-cattle-outbreak trees (n=9) locally.

@@ -124,7 +124,7 @@
"map_triplicate": false,
"color_by": "host",
"geo_resolution": "division",
"distance_measure": "div"
"distance_measure": "__VALUE_OVERWRITTEN_BY_SNAKEMAKE_PIPELINE__"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not asking for a change, just thinking through how this value can affect UX

Interesting how this tightly couples the Auspice config with this version of the workflow. If the auspice_config rule is changed in a way that fails to edit this value, the export rule will run into a validation error, which is a great check for us. On the other hand, if someone tries to use this config in another workflow (or older version of this workflow), they will also run into a validation error.

Copy link
Member Author

Choose a reason for hiding this comment

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

the export rule will run into a validation error

This was my thinking for this decision. I could have left it as "div" but wanted to avoid silent failures. Of the two downsides to this approach you point out,

if someone tries to use this config in another workflow

this is fair, although hopefully the text is a big enough hint to them that they have to add something on the snakemake side! They'll get validation errors if they forget

or older version of this workflow

given both the config and the snakemake pipeline are in version control I expect such usage to never happen, but 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

or older version of this workflow

given both the config and the snakemake pipeline are in version control I expect such usage to never happen, but 🤷

Mostly thinking of people who copy/paste configs without being fully aware of these internal nuances 🤷‍♀️

But yeah, I think the validation error is clear enough in this case

Validating schema of 'results/h5n1-cattle-outbreak/ha/default/auspice-config.json'...
  .display_defaults.distance_measure "__VALUE_OVERWRITTEN_BY_SNAKEMAKE_PIPELINE__" failed enum validation for ["num_date", "div"]

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking through things Jover!

@jameshadfield jameshadfield merged commit 0b5dcae into master Oct 29, 2024
6 checks passed
@jameshadfield jameshadfield deleted the james/dynamic-auspice-config branch October 29, 2024 20:52
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.

Restore timetree default to H5N1 cattle outbreak genome analysis
2 participants