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

Type cast continuous values in tree #1655

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Conversation

jameshadfield
Copy link
Member

Conceptually these values only make sense if they are numeric. Previously some implicit casting was happening, but this resulted in bugs such as negative values not being displayed in the scatterplot.

Note one slight side-effect which may be considered a regression: previously string values which were not numbers (e.g. "abc" but not "1.0") would show up in the hover/click info-boxes. Now such values will be changed to undefined and therefore not shown in those info-boxes.

Timing of the changes introduced here (AppleM1, Firefox 111):

  • nextclade/sars-cov-2/21L 7-9ms
  • ncov/gisaid/global/6m 4-10ms
  • zika 2ms

Closes #1626

@corneliusroemer
Copy link
Member

corneliusroemer commented Mar 23, 2023

Thanks for working on this!

It doesn't fix the issue in my test (maybe I'm testing wrong?)

Repro

  1. Change working directory to auspice repo
  2. Run:
gh pr checkout 1655
mkdir tmp
curl https://gist.githubusercontent.com/corneliusroemer/ecf99b3ba5bce5aef57a5538ac50da20/raw/98099bf349c50b7c23d73d58fc6e1a4d46744287/auspice.json -o tmp/nextclade_21L.json
npm run view -- --datasetDir tmp       
  1. Then open in browser: http://localhost:4000/nextclade/21L?branches=hide&c=ace2_binding&l=scatter&p=full&scatterX=ace2_binding&scatterY=immune_escape

See results (still not showing negative):
image

Maybe I'm testing wrong, see #1656

@tsibley
Copy link
Member

tsibley commented Mar 23, 2023

@corneliusroemer You'll need to npm run build when switching branches. npm run view doesn't automatically rebuild (it's not like cargo run, as I just noted elsewhere). Alternatively, you can npm run dev instead of npm run view to use a auto-rebuilding, hot-reloading version.

@tsibley
Copy link
Member

tsibley commented Mar 23, 2023

The README does mention this briefly, but uses auspice develop instead of npm run dev since the install docs for development suggest a Conda environment with Node + npm install --global . for Auspice.

@corneliusroemer
Copy link
Member

corneliusroemer commented Mar 23, 2023

Thanks @tsibley! I'm still struggling unfortunately.

Can you adapt this script to show me how to use what you suggest?

This:

npm run build
npm run view -- --datasetDir tmp 

doesn't seem to work.

Neither does

npm run dev -- --datasetDir tmp  

Edit: I'm getting errors building, see #1656 (comment)

@tsibley
Copy link
Member

tsibley commented Mar 23, 2023

This … doesn't seem to work.

You'll need to say more about what's not working…

@corneliusroemer
Copy link
Member

Sorry, I didn't see the error at first: getting build error, see #1656 (comment)

@corneliusroemer
Copy link
Member

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Thanks @jameshadfield! Reviewing this forced me to read more about isNaN and Number. Never realized they worked in such unexpected ways.

src/util/castJsonTypes.js Outdated Show resolved Hide resolved
src/util/castJsonTypes.js Outdated Show resolved Hide resolved
src/util/castJsonTypes.js Outdated Show resolved Hide resolved
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-continous-scale-cwwcar March 27, 2023 00:57 Inactive
Conceptually these values only make sense if they are numeric.
Previously some implicit casting was happening, but this resulted in
bugs such as negative values not being cast and thus not displayed in
the scatterplot.

Note one slight side-effect which may be considered a regression:
previously string values which were not numbers (e.g. "abc" but not
"1.0") would show up in the hover/click info-boxes. Now such values will
be changed to `undefined` and therefore not shown in those info-boxes.

Timing of the changes introduced here (AppleM1, Firefox 111):
* nextclade/sars-cov-2/21L   7-9ms
* ncov/gisaid/global/6m      4-10ms
* zika                       2ms

Closes #1626
@jameshadfield jameshadfield merged commit 4b06389 into master Jun 6, 2023
@jameshadfield jameshadfield deleted the continous-scale-fixes branch June 6, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Scatterplot not displaying negative values for ACE2 binding
5 participants