[export] reduce numeric precision to reduce dataset size by ~30% #1512
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1512 +/- ##
==========================================
+ Coverage 69.90% 70.09% +0.19%
==========================================
Files 75 75
Lines 7888 7923 +35
Branches 1933 1938 +5
==========================================
+ Hits 5514 5554 +40
+ Misses 2088 2085 -3
+ Partials 286 284 -2 ☔ View full report in Codecov by Sentry. |
genehack
reviewed
Jul 2, 2024
joverlee521
requested changes
Jul 2, 2024
jameshadfield
force-pushed
the
james/reduce-numeric-accuracy
branch
from
68a28d7
to
d1b0cca
Compare
|
Updated the code a bit and added unit tests |
genehack
approved these changes
Jul 8, 2024
joverlee521
approved these changes
Jul 9, 2024
jameshadfield
added a commit
that referenced
this pull request
Jul 10, 2024
Based on PR feedback in <#1512 (comment)> This better conveys that confidences for numeric traits must have 2 and only 2 elements.
jameshadfield
force-pushed
the
james/reduce-numeric-accuracy
branch
from
d1b0cca
to
3f2f82c
Compare
genehack
approved these changes
Jul 10, 2024
victorlin
reviewed
Jul 15, 2024
Using fewer sig figs (or decimal places, as appropriate) is just fine for Auspice's usage and helps reduce the output JSON size. Testing on the zika dataset this reduces the (minified, gzipped) JSON by 20% from 175kB to 139kB. This refactor also uses more thorough error checking and enforcement that entropy values are found together with confidence values where appropriate. (Note that the previous usage of `is_valid` was bogus as it only works for string values.)
This extends the work in the previous commit to reduce the sig. figs / decimal places of all numeric node attrs. The file size of the zika dataset is reduced to 120kB which, when combined with the previous commit, is a 31% reduction cf. Augur 24.4.0
The previous restriction to the highest 4 values was motivated by keeping the eventual Auspice dataset small. That restriction is now part of `augur export v2` so we can now report them all here. This results in slightly larger node-data files (a 1.5% increase for the zika analysis) but will produce more thorough data for any scripts / non-Auspice usage.
Based on PR feedback in <#1512 (comment)> This better conveys that confidences for numeric traits must have 2 and only 2 elements.
jameshadfield
force-pushed
the
james/reduce-numeric-accuracy
branch
from
3f2f82c
to
5f1a538
Compare
|
Docs CI failures due to sphinx-doc/sphinx-argparse#56, but all tests pass. Merging this PR now as per this advice on Slack:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Reduces the precision of numerical values in the Auspice JSON (node attrs, confidence & entropy values) whilst keeping enough precision so that the rendering / display in Auspice is unchanged.
Motivated by this slack thread
It'd be good to get a few different eyes on this PR as it affects every single dataset Augur produces.
Related work
The output of
augur traitsdoesn't currently distinguish between a (terminal) node with a provided value vs an inferred one. They both look something like{ "value": "foo", "confidence": {"foo": 1.0}, "entropy": -1.000088900581841e-12 }We could reduce the JSON size a bit more if we were able to know the value wasn't inferred and thus drop the confidence & entropy fields from the export. (This may require some small Auspice changes as well.)
Checklist