-
Notifications
You must be signed in to change notification settings - Fork 3
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
Added units to plots that can produce relative values #49
Merged
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
…itment, and spawning biomass) as per #44
…; updated fxns with this arg (plot_biomass, recruitment, ssb); moved code that computes min/max B, R, ssb from write_captions to add_more_key_quants; updated add_more_key_quants to label scaled values with appropriate label that's inclusive of scale's order of magnitude
…nts into captions/alt text csv; updated exp_all_figs_tables to include new arguments specifying scaling amounts for biomass, recruitment, and sp biomass plots
…ddress failing tests
Schiano-NOAA
approved these changes
Feb 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I have some minor suggestions you can adopt or not. I really like how you identified the scaling and how it will change the units
Schiano-NOAA
pushed a commit
that referenced
this pull request
Feb 24, 2025
* Added units to plots that can produce relative values (biomass, recruitment, and spawning biomass) as per #44 * Added new arg to add_more_key_quants to allow for values to be scaled; updated fxns with this arg (plot_biomass, recruitment, ssb); moved code that computes min/max B, R, ssb from write_captions to add_more_key_quants; updated add_more_key_quants to label scaled values with appropriate label that's inclusive of scale's order of magnitude * Added code that adds new key quantities extracted in add_more_key_quants into captions/alt text csv; updated exp_all_figs_tables to include new arguments specifying scaling amounts for biomass, recruitment, and sp biomass plots * Fix test * Update documentation for exp_all_figs_tables * Attempt to fix test failures by fixing path in add_more_key_quants() * add dat arg to B, R, SSB plotting fxns, and add_more_key_quants, to address failing tests * Updates per Schiano-NOAA's comments on PR (minor edits to code, documentation)
Schiano-NOAA
added a commit
that referenced
this pull request
Feb 24, 2025
* change package name * Added units to plots that can produce relative values (#49) * Added units to plots that can produce relative values (biomass, recruitment, and spawning biomass) as per #44 * Added new arg to add_more_key_quants to allow for values to be scaled; updated fxns with this arg (plot_biomass, recruitment, ssb); moved code that computes min/max B, R, ssb from write_captions to add_more_key_quants; updated add_more_key_quants to label scaled values with appropriate label that's inclusive of scale's order of magnitude * Added code that adds new key quantities extracted in add_more_key_quants into captions/alt text csv; updated exp_all_figs_tables to include new arguments specifying scaling amounts for biomass, recruitment, and sp biomass plots * Fix test * Update documentation for exp_all_figs_tables * Attempt to fix test failures by fixing path in add_more_key_quants() * add dat arg to B, R, SSB plotting fxns, and add_more_key_quants, to address failing tests * Updates per Schiano-NOAA's comments on PR (minor edits to code, documentation) * add warning for previous naming * add callout box for name change --------- Co-authored-by: Sophie Breitbart <[email protected]>
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.
NOTE: After writing the text below, I realized that the issue #44 was written confusingly (by me 😄). The "relative" argument is not the same thing as the "scaling" argument. I'll just keep this PR going, as a draft, and turn my attention toward adjusting the alt text to adapt to the scaling argument.
Specifically edited the alt text for relative biomass, recruitment, and spawning biomass as per #44.
I think all that's needed to satisfy that issue is to simply state the units (which weren't stated before in these figures' alt texts; only the captions). The key quantities are being computed correctly, and since the alt text doesn't state the axis units- only the min and max values- I believe it's fine to keep using the regular plots' units for these figures.
For example, this is the alt text for relative recruitment:
Line graph showing the assessment model estimated relative recruitment in sr.units for each year. The x axis shows year, which spans from recruitment.start.year to recruitment.end.year . The y axis shows relative recruitment (R/R
0), which spans from rel.recruitment.min to rel.recruitment.max recruitment.units . (I just added the bolded units placeholder in this PR).If the min and max recruitment are 1,000 and 1,000,000 pounds, then the relative values (R/R0) (could be) 1-1,000 pounds. This alt text would still be sufficient for the relative case:
"The y axis shows relative recruitment (R/R
0), which spans from 1 to 1,000 pounds."