Plot tweaks

[ArtPoon]

• in general, modifications to region plots (region-specific variant frequencies over time)
• replaced monotonic smoothing of lines with default linear mode to avoid over-smoothing
• added points (CustomizedDot) representing observed variant frequencies, with area scaled to observed counts
• added "lollipop stems" connecting points to trend lines as visual cue
• replaced default ActiveDot with CustomizedActiveDot to draw line segments representing confidence intervals

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
flu-frequencies	✅ Ready (Inspect)	Visit Preview	May 15, 2023 11:51am

[ivan-aksamentov]

Thanks for inviting me for a review!

I am not in the loop with regards to the science of this work and as an engineer I am only competent to comment on the technical side of things. I will let Richard and Cornelius to comment on scientific soundness, utility and usability of the feature.

Technically though, it's a good start and it seems to be displaying nice bubbles, however, there is a few problems I noticed:

• The PR does not have any description. I don't know what was the goal and what have been done exactly. So it's difficult for me to make a review.

• CustomizedDot and CustomizedActiveDot components don't need to be nested inside the plot component. We could put them into a separate file or two.

• It is recommended to use plain function syntax function MyComponent() { ... } for most places, instead of arrow functions const MyComponent = () => { ... }.

• Would be nice to avoid defining multiple variables with the comma operator. It's hard to read and is error prone. A const on every line is fine.

• Use const where possible (instead of let)

• Lots of magic numbers and operations without explanation. Some more comments and/or extracting formulas into named functions, and numbers into named constants would be nice.

The linter and type checker are disabled in the PR deployments currently, but there are probably more things they will catch. Here is how to run them locally:

yarn run lint

automated fixes can be applied with:

yarn run lint:fix

automated code formatting:

yarn format:fix

There will be type errors. For example type any is not allowed. Typing the props for the dot components is difficult. I will adjust it later.

Let me know if there's technical troubles I can help to resolve.

I suggest to add Richard and/or Cornelius to the reviewers or to chat with them in person, to discuss the science of things (if not already).

[ArtPoon]

Thanks @ivan-aksamentov - I've since found some bugs that arise when the frequency estimate is 1 (causes a division by zero error because I'm relying on this quantity to scale to the plot region)

[ivan-aksamentov]

@ArtPoon By Richard's request I just merged branch web to master. So the PRs would now need to go to master.

[ArtPoon]

Do you guys use a primary dev branch or can a PR come from any branch?

[ivan-aksamentov]

@ArtPoon master branch is the dev branch. For web apps we typically do the following 3-branch strategy:

• for each PR, create a feature branch stemming from master. Develop features there. PRs are deployed by Vercel bot automatically into a throw-away "preview" environment (link is in the bot's message)
• merge PRs into master, accumulate new features there
• CI automatically deploys from master branch into master environment, for example for Nextclade this is https://master.clades.nextstrain.org
• before new release, we merge master branch into staging branch; CI automatically deploys it to https://staging.clades.nextstrain.org and we test things there for a while
• when ready, we merge to release branch and CI automatically deploys to it https://clades.nextstrain.org, where end users can find it

(examples are with Nextclade because flu_frequencies does not have deployment environments or even domain name yet)

[ivan-aksamentov]

I made a few refactorings to address linter warnings and improve type-safety (hopefully). Technically this looks good to me.

However, now that I learned how it works, I find the UX a bit strange. The bubbles introduce quite a bit of visual load, and they are displayed unconditionally. Then the bubbles are filled on mouse hover when the ranges are not shown, but not filled when ranges are shown. Similarly, the lines are shown in one mode and not another.

I understand that hollow bubbles allow to see through them better. But this all seems quite random to me. As a user I would not be able to tell what to expect every time I hover a mouse and why it is done this way.

I propose to consider (perhaps in followup PRs) to add checkboxes or switch toggles to enable and disable the bubbles and confidence lines independently, without attaching this to one another and to confidence ranges (they will also stay independent). Some users have very large displays and they browse in full screen mode, so they can have preferences different from someone browsing from a phone. Ability to configure plot components independently would offload UX decisions away from us to the end users.

Additionally, in this application we have 2 types of plots: for regions and for variants. Which are really the same plot, but with different data. For symmetry, is the bubbles and lines something that also makes sense in the other type of plots?

In any case, I will let Richard and Cornelius to comment on the scientific side and UX of this feature as well as its further extensions, and to make the final decision.

[ArtPoon]

Okay fair point about giving users the option to turn off the bubbles. I will have line segments displayed whether or not shaded regions are being displayed instead of switching to filled cirlces, i.e., previous behaviour. I was going to port these interface changes to the variant-specific plot after drafting the implementation in the region-specific plots.

[ivan-aksamentov]

@ArtPoon In #16 I also scaffolded the checkboxes and the state required for it, in case you want to go this way. Feel free to take over and use all of it, or a part of it or none of it.

[ArtPoon]

@ArtPoon In #16 I also scaffolded the checkboxes and the state required for it, in case you want to go this way. Feel free to take over and use all of it, or a part of it or none of it.

Well shoot, I just implemented checkboxes already and didn't see your message :-(

[ivan-aksamentov]

@ArtPoon Haha! No worries!

[ArtPoon]

I'm trying to resolve two next-dev.js errors that are thrown when loading a region or variant page, respectively. For region page, we get the following:

Warning: Can't perform a React state update on a component that hasn't mounted yet. This indicates that you have a side-effect in your render function that asynchronously later calls tries to update the component. Move this work to useEffect instead.

This seems to be associated with the following line:
https://github.com/neherlab/flu_frequencies/blob/877f244c97a76151a5fe03ceecea5f3e08761c0e/web/src/state/variants.state.tsx#L20-L29C3

[ArtPoon]

Hm, never mind - I'm getting the same errors on master branch, so it doesn't seem to be my fault :-)

[rneher]

thanks, Art. This looks great to me.

[ArtPoon]

Thanks @rneher! @ivan-aksamentov is this ok to merge?