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

update broadinstitute/auspice:master from upstream nextstrain/auspice:master #7

Merged
merged 101 commits into from
Jul 6, 2021

Conversation

tomkinsc
Copy link
Member

@tomkinsc tomkinsc commented Jul 6, 2021

No description provided.

trvrb and others added 30 commits January 26, 2021 21:41
This commit forces controls.normalizeFrequencies to be false if there are any pivots where the total frequency is less than 0.1%.

This addresses two issues:
1. In the existing code, attempting to normalize situations where pivots have 0% total frequency results in bad looking "all equal" bands. This commit removes the capacity to get into this bad looking app state.
2. When filtering to a particular clade, we often want to switch to unnormalized frequencies anyway (as opposed to filtering to geography). This commit accomplishes this automatically because filtering to an emerging clade will generally result in pivots with <0.1% frequency.
Push each value individually instead of all at once, which results in
more method calls but a much smaller call stack size.  Alternatively,
Array.concat could be used, but this follows the pattern of surrounding
code and avoids reassignment of "options", which would also necessitate
removal of the "const" declaration.

The /tb/global build has ~277k genotype states, which resulted in a call
to Array.push with as many arguments when the spread syntax was used.
This blew through the call stack size limit on Chrome with an error
like:

    RangeError: Maximum call stack size exceeded
        at FilterData.eval [as makeOptions] (filter.js?6bcb:65)
        at FilterData.render (filter.js?6bcb:100)
        …

Firefox was unaffected, so presumably has a larger limit.

Debugging was waylaid for a bit by the assumption that exceeding the
call stack size necessarily meant deep recursion, but the lack of a deep
stack trace led to the realization that it can also occur when a
function's arguments are too many.

Resolves #1292.
controls/filter: Avoid spread syntax with potentially large arrays
BCE dates were correctly interpreted but incorrectly rendered
due to a bug in the final string-prettying step. This would
result in a tree with the correct layout and positioning, but
incorrect labels ("-undefined"). This commit remedies this and
adds a test.
This simply increases the limits to allow CI tests to pass.
Large bundlesizes are a long-term concern, but given the
recent bugs regarding bundling and other priorities this change
essentially pushes improvements here to "sometime in the future"
The function `collectMutations` now doesn't report reversions (where the tip state = the ancestral state) and combines multiple mutations (e.g. A->B->C is now A->C rather than two separate mutations).
Previous implementation continued to display the toggle icon but disabled its functionality (by forcing `normalizeFrequencies` to be `false`). Here we replace the toggle with a "not available" message & update the info-popup text.
Upon initial parsing of the frequencies JSON as well as frequency data updates we may set redux→controls→normalizeFrequencies→false. This commit modifies the LOAD_FREQUENCIES and FREQUENCY_MATRIX actions to pass this information to the reducer, rather than updating the redux state directly from within the actions.

I couldn't find any bugs caused by the previous implementation, but this change is more in line with suggested behaviour and should help future-proof work here.
The frequencies component performed some basic comparisons between the previously rendered data and new data to avoid unnecessary re-renders. This logic was too simplistic and caused a bug where the component wouldn't re-render the graph when the data had indeed changed. This commit skips these checks. This may result in some unnecessary re-renders, however I couldn't find any in my testing.

Closes #1224
When a frequency value is below 1% we now display "<1%" rather than rounding to the nearest integer which could lead to confusing output of  "frequency: 0%".

Closes #1279
Shifts the top of the tree down slightly so that tips and branches cannot be hidden behind the (closed) legend, which prevents interacting with them. This only happens in rectangular / unrooted trees, as radial / clock views almost never have tips rendered in the top-left corner.
Show root-to-tip mutations in tip-clicked info box
Legend no longer obscures branches/tips.
This is a squashed & rebased version of PR #1221,
which itself superseded PR #1218.

Closes #1049.

Co-authored-by: Charlie Jones <[email protected]>
Co-authored-by: eharkins <[email protected]>
Allow JSONs to define language
Enforce unnormalized frequencies when data is lacking
didithilmy and others added 29 commits May 10, 2021 16:12
This action will run on each auspice PR and create a corresponding
PR on nextstrain.org which includes a commit using the version
of auspice from this (auspice) PR. This functionality is extremely
useful for auspice development as it will allow us to use a Heroku
review app to test auspice in the context of nextstrain.org

There are a number of future improvements to implement:

* New auspice releases (tagged commits on `release` branch)
would ideally create a PR on nextstrain.org which could be merged
to update the version of auspice there.

* Other consumers of auspice (e.g. auspice.us) could be added to this
GitHub Action.
GitHub Action to create nextstrain.org PR
This implements a requested improvement to the original
scatterplot implementation. The implementation hinges on two changes:
(1) The collection of values for a given variable (e.g. x-var) need
to be computed and passed to PhyloTree to act as the scale's domain.
We reuse the colorScale machinery here, which could be optimised
(see todo messages in code), but this has the advantage that the
domain ordering matches the legend (unless user supplied).
(2) PhyloTree needed to be modified to use non-linear scales, in this
case `pointScale`.

This commit should be fully functional, however there are some
future improvements to be made:

(i) Grid text is obscured and unreadable when there are many entries
in the domain.
(ii) Genotypes and Boolean scales are not yet available.
(iii) Jitter should be added to nodes to avoid obfuscation.
This commit is in preparation for allowing genotype to be a scatterplot
variable. This will complicate the allowable scatterplot variables
and force these to update upon colorBy changes. This is much cleaner
if layout is changed in a thunk.
Genotype is treated differently to other colorings in two important
ways: (1) it can change value, for instance when changing the
colorby to another genotype position and (2) it is stored in a
different place to other colorings. These require scatterplot logic
to be more complex as actions are no longer separate - we now require
a NEW_COLOURS action to potentially update the layout which was
formerly within the remit of the CHANGE_LAYOUT actions. This is
achieved through a middleware layer.

This implementation makes it clear that jitter and better domain
spacing are crucial for scatterplots.
This prevents nodes falling on the axis itself or at the very end of
the grid, which was especially noticeable for traits with small domains.
Allow non-continuous scatterplot variables
We have had issues in the past with legend values from column 1 overflowing into column 2. For instance, issue #899 was fixed by PR #914 which implemented a maximum character limit for legend names. This solution can produce misleading views, such as those described in #1306.

This solution implements a clipping mask for column 1, avoiding the complication of limiting the string size. Column 2 already has similar behaviour because the SVG element of the legend itself performs the clipping.
Apply clipping to first column of legend
Use filterOptions to modify filter search algorithm
Fixes a bug where the ability to toggle regression lines was hidden for clock views. (The ability to hide this toggle is only intended for scatter layouts, where we should not expose the toggle unless both axes are showing continuous variables.)
Always show regression toggle for clock layout
The existing grayscale color ramp (used for values absent in an explicitly specified color scale) had values that were too dark and threw off the overall color balance. This commit narrows the grayscale color ramp to be more in line with pastel color ramp.
This adds a bit of blue into the grayscale color ramp. Still reads as mostly gray, but no colors seem to exist more in the same universe as canonical auspice color ramp.
Styling adjustments to footer text
This commit uses dataProvenance in metadata to identify datasets using "GISAID" data. For these datasets, the full metadata download is swapped to an "acknowledgments" download that only includes the following fields:
 - strain
 - gisaid_epi_isl
 - genbank_accession
 - originating_lab
 - submitting_lab
 - author
This commit cleans up naming of metadata headers in downloaded metadata TSV. It does the following:
1. Keeps headers as input into "augur export" rather than renaming by title. Thus it has "originating_lab" rather than "Originating lab", "pango_lineage" rather than "PANGO lineage", etc... This should make it easier for people to process downloaded metadata from Auspice alongside metadata provisioned by Nextstrain (via GISAID or via S3).
2. Makes "date" the second column as this is often what's most important. I couldn't figure out a way to intelligently order remaining fields. My first thought was to use metadata.colorings, but this isn't sorted.
3. Fixes "accession". It had been exporting as "[object Object]".
Remove metadata download from GISAID datasets
@tomkinsc tomkinsc merged commit 7e4bc92 into broadinstitute:master Jul 6, 2021
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.

6 participants