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

More control over size histogram #72

Merged
merged 13 commits into from
Jan 15, 2025
Merged

More control over size histogram #72

merged 13 commits into from
Jan 15, 2025

Conversation

afmagee42
Copy link
Contributor

Resolves, more or less, #38 by giving users control over what they want the histogram to show. Users can now toggle between cumulative sizes and sizes in specific generations for any generation simulated.

Bonus out-of-scope bug fix: app.render_percents() was rounding wrong.

@afmagee42 afmagee42 requested a review from swo January 8, 2025 18:20
@afmagee42 afmagee42 linked an issue Jan 10, 2025 that may be closed by this pull request
Copy link
Contributor

@swo swo left a comment

Choose a reason for hiding this comment

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

A bunch of small thoughts:

  • I think it's easier to have the plot options next to the "Advanced options"? I like the sidebar having all the controls, and the main window being just results displayed
  • Rather than changing the text above the plot (to adapt for # of generations and cumulative vs. not), I think it would be easier to have that be the x-axis title of the plot itself. Fewer places to look
  • Weirdly, the altair plot doesn't break at integers. If this is easy to fix (or you think is worth breaking off into another issue that I tackle), we could look into it.
    image

ringvax/app.py Show resolved Hide resolved
ringvax/app.py Outdated Show resolved Hide resolved
ringvax/app.py Outdated Show resolved Hide resolved
ringvax/summary.py Outdated Show resolved Hide resolved
ringvax/summary.py Outdated Show resolved Hide resolved
ringvax/summary.py Show resolved Hide resolved
ringvax/app.py Outdated Show resolved Hide resolved
@afmagee42
Copy link
Contributor Author

Weirdly, the altair plot doesn't break at integers. If this is easy to fix (or you think is worth breaking off into another issue that I tackle), we could look into it.

The breaks on these altair histograms have annoyed me since #32 but I think better breaks is a follow-up issue, including:

  • Having 0 not be it's own category means you can't see the extinct generations separately from the small ones
  • Should we just show every integer value in the range?
  • Should we try to keep ranges comparable across all possible subsets here?

Rather than changing the text above the plot (to adapt for # of generations and cumulative vs. not), I think it would be easier to have that be the x-axis title of the plot itself. Fewer places to look

I previously tried an altair title, and it was both too long (got chopped off) and mucked up the size of the displayed histogram. Now trying it as an x axis label, I see it merely leads to a truncation issue. If we can workshop a shorter (looks like 65 characters or less) and equally clear option, I am game for this.

  • "Number of infections in generation G" seems clear for the one, and fits
  • "Cumulative infections in generations up to and including" is just too long for the other

@swo
Copy link
Contributor

swo commented Jan 13, 2025

  • "Cumulative infections in generations up to and including" is just too long for the other

"Cumulative infections through generation"?

@afmagee42 afmagee42 requested a review from swo January 14, 2025 18:57
Copy link
Contributor

@swo swo left a comment

Choose a reason for hiding this comment

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

Amazing. It even has a test!

ringvax/app.py Outdated Show resolved Hide resolved
Co-authored-by: Scott Olesen <[email protected]>
@afmagee42 afmagee42 merged commit 7fef0a5 into main Jan 15, 2025
2 checks passed
@afmagee42 afmagee42 deleted the afm-38 branch January 15, 2025 17:08
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.

Refine plots of numbers of infections
2 participants