Allow multiple thresholds for measurements #1148

joverlee521 · 2023-02-02T01:47:01Z

Description of proposed changes

Adds the ability to export multiple thresholds for the measurements JSON.

Note that this will produce measurements JSONs that will only have the
"thresholds" array but not the deprecated single "threshold" value. This
means the thresholds will not be displayed in Auspice v2.42.0 or lower
because Auspice still expects the single "threshold" value.

Testing

Added additional Cram test for multiple threshold values.

Checklist

Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file.

codecov · 2023-02-02T02:01:26Z

Codecov Report

Base: 68.20% // Head: 68.17% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (e9ddbcb) compared to base (ebdcd23).
Patch coverage: 20.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
- Coverage   68.20%   68.17%   -0.04%     
==========================================
  Files          63       63              
  Lines        6749     6752       +3     
  Branches     1653     1654       +1     
==========================================
  Hits         4603     4603              
- Misses       1838     1841       +3     
  Partials      308      308

Impacted Files	Coverage Δ
augur/measurements/export.py	`29.52% <20.00%> (-0.87%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tsibley · 2023-02-03T01:02:12Z

augur/measurements/export.py

+    # Convert deprecated single threshold value to list if "thresholds" not provided
+    single_threshold = collection_config.pop("threshold", None)
+    if single_threshold is not None and "thresholds" not in collection_config:
+        collection_config["thresholds"] = [single_threshold]


I'm not clear on the expected deprecation/support story here with regards to Auspice. There's no existing Auspice version that uses thresholds but with this Augur will only produce thresholds, thus making them talk past each other? It seems like this arrangement is likely to lead to harsh compatibility constraints, even once there is a release of Auspice that uses thresholds.

It seems to me that Augur could instead produce threshold as a single number when only one threshold is provided, thus only raising compatibility issues if multiple thresholds are actually provided. Then any existing usage with a single threshold (i.e. all uses of it currently, right?) won't have any compatibility issues with Augur and Auspice.

Yeah, @huddlej and I had a brief conversation about this in Slack.

My thought here is that we will want to drop the deprecated threshold key as some point, so might as well do it now when there aren't that many (known) users of the command/feature.

continuing this discussion in another thread (probably shouldn't have split the two ideas in the first place)

tsibley · 2023-02-03T01:04:04Z

augur/data/schema-measurements.json

+                    "thresholds": {
+                        "description": "Numeric measurement thresholds to be displayed as solid grey lines across subplots. Optional -- if not provided, no threshold will be displayed.",
+                        "type": "array",
+                        "minItems": 1,
+                        "items": {
+                            "description": "A numeric measurement threshold value.",
+                            "type": "number"
+                        }
+                    },
                    "threshold": {
-                        "description": "A numeric measurement threshold to be displayed as a single grey line shared across subplots. Optional -- if not provided, no threshold will be displayed",
+                        "deprecated": true,
+                        "description": "DEPRECATED - A numeric measurement threshold to be displayed as a single grey line shared across subplots. Will be ignored if 'thresholds' is provided.",


Instead of deprecating one key and adding another, did you consider making the existing threshold key take either a single number or an array of numbers? This doesn't solve the compatibility issue with Auspice, but it does simplify the schema a bit.

I guess it does mean that Auspice may barf on threshold: […] instead of merely ignoring thresholds: […]. Not entirely sure how gracefully it'd handle that type change, but I think it'd work out because of the type test for whether to display the toggle or not and that x1/x2 for the SVG line would get set to NaN.

I considered it at first, but thought having two separate keys would make deprecating the single value thresholds easier to explain.

(from another comment)

My thought here is that we will want to drop the deprecated threshold key as some point

Ah, my thinking was that we wouldn't have to drop support for a single int and would indefinitely accept both a single value and list of values, e.g., threshold: x and threshold: [x, y, z].

Per my other comment, Augur would then continue to produce threshold: x when only a single value is given.

This combination maximizes compatibility without much effort on our part.

IMO, the "deprecation" of threshold vs. thresholds is unfortunate, because it's not really a deprecation, it's a removal: after version X, Augur won't produce it and thus if you have older Auspice, you're stuck on the Augur version prior to X (or stuck twiddling the final JSON manually).

augur/measurements/export.py

Marks the previous single value threshold as deprecated, but is still supported. The single value threshold will be ignored if the multiple value thresholds array is provided.

Updates the option `--threshold` to accept multiple values. If a single "threshold" value is provided via the `--collection-config`, it will be converted to a "thresholds" array in the measurements JSON. Note, that this will produce measurements JSONs that will only have the "thresholds" array but not the deprecated single "threshold" value. This means the thresholds will not be displayed in Auspice v2.42.0 or lower because Auspice still expects the single "threshold" value.

The default argparse abbreviation matching¹ allows us to update the option to `--thresholds` to match the key in the produced JSON while still supporting the previous single `--threshold` option. Updated one of the Cram tests to use the plural form while leaving the others in their original singular form to ensure both work as expected. ¹ https://docs.python.org/3.7/library/argparse.html#argument-abbreviations-prefix-matching

huddlej

Thank you for all of your work on this, @joverlee521! This looks good to me with a minor text edit exception mentioned below. I agree with @tsibley that there could be some painful version mismatches between Augur and Auspice, but I'm slightly ok with this because:

we will at least release this change as a major/breaking release of Augur
new Auspice will gracefully handle older measurements JSONs we've already generated and the new ones
when there is a mismatch where old Auspice gets thresholds from new Augur, the thresholds will completely disappear from the measurements panel which is an easier bug to detect than somehow falling back to one threshold to display, etc.

The first two points should mean that anyone who uses the Docker image or managed Conda environment won't notice anything when they upgrade. The users who will experience this most will be folks like many on our team who have "ambient" installations of both Augur and Auspice and who will need to remember to update both. Maybe to support these folks, we should include a specific note in the CHANGES of this PR's major release that users will want to update Auspice at the same time they update to Augur 21.0.0.

augur/measurements/export.py

Reflect the change that the `--show-threshold` option will apply to one or multiple thresholds.

tsibley

Added a few responses up thread and a few thoughts below, but none blocking. 👍 for merge at will.

To @huddlej's comments:

we will at least release this change as a major/breaking release of Augur

It's a good practice, and useful for some downstream folks, but most of the time major versions for breaking changes is probably really only cover for "well, we told you it would break". ;-)

The first two points should mean that anyone who uses the Docker image or managed Conda environment won't notice anything when they upgrade.

This is a good point, esp. as we've pushed more people to those managed runtimes. It's a good mitigation.

Maybe to support these folks, we should include a specific note in the CHANGES of this PR's major release that users will want to update Auspice at the same time they update to Augur 21.0.0.

That'd be great!

joverlee521 · 2023-02-07T23:36:25Z

Rebased onto master and added entry to changelog specify that users will need to update to Auspice v2.43.0 for thresholds to be properly displayed.

joverlee521 requested a review from huddlej February 2, 2023 01:47

joverlee521 force-pushed the multi-threshold-measurements branch from 9015dd3 to 3a0af9f Compare February 2, 2023 22:52

tsibley reviewed Feb 3, 2023

View reviewed changes

joverlee521 mentioned this pull request Feb 3, 2023

Measurements paper improvements nextstrain/auspice#1631

Merged

2 tasks

joverlee521 added 3 commits February 6, 2023 17:00

Update measurements schemas to allow multiple thresholds

514dd51

Marks the previous single value threshold as deprecated, but is still supported. The single value threshold will be ignored if the multiple value thresholds array is provided.

huddlej approved these changes Feb 7, 2023

View reviewed changes

augur/measurements/export.py Show resolved Hide resolved

measurements export: update help text for --show-threshold

c43f808

Reflect the change that the `--show-threshold` option will apply to one or multiple thresholds.

tsibley approved these changes Feb 7, 2023

View reviewed changes

Update changelog

e9ddbcb

joverlee521 force-pushed the multi-threshold-measurements branch from 9eec326 to e9ddbcb Compare February 7, 2023 20:05

joverlee521 merged commit 03a9188 into master Feb 7, 2023

joverlee521 deleted the multi-threshold-measurements branch February 7, 2023 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple thresholds for measurements #1148

Allow multiple thresholds for measurements #1148

joverlee521 commented Feb 2, 2023 •

edited

Loading

codecov bot commented Feb 2, 2023 •

edited

Loading

tsibley Feb 3, 2023

joverlee521 Feb 3, 2023

tsibley Feb 7, 2023

tsibley Feb 3, 2023

joverlee521 Feb 3, 2023

tsibley Feb 7, 2023 •

edited

Loading

huddlej left a comment

tsibley left a comment •

edited

Loading

joverlee521 commented Feb 7, 2023

Allow multiple thresholds for measurements #1148

Allow multiple thresholds for measurements #1148

Conversation

joverlee521 commented Feb 2, 2023 • edited Loading

Description of proposed changes

Testing

Checklist

codecov bot commented Feb 2, 2023 • edited Loading

Codecov Report

tsibley Feb 3, 2023

Choose a reason for hiding this comment

joverlee521 Feb 3, 2023

Choose a reason for hiding this comment

tsibley Feb 7, 2023

Choose a reason for hiding this comment

tsibley Feb 3, 2023

Choose a reason for hiding this comment

joverlee521 Feb 3, 2023

Choose a reason for hiding this comment

tsibley Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

huddlej left a comment

Choose a reason for hiding this comment

tsibley left a comment • edited Loading

Choose a reason for hiding this comment

joverlee521 commented Feb 7, 2023

joverlee521 commented Feb 2, 2023 •

edited

Loading

codecov bot commented Feb 2, 2023 •

edited

Loading

tsibley Feb 7, 2023 •

edited

Loading

tsibley left a comment •

edited

Loading