Skip to content

Comments

Plot updates#260

Closed
stephanie-owen wants to merge 14 commits intodevfrom
plot-updates
Closed

Plot updates#260
stephanie-owen wants to merge 14 commits intodevfrom
plot-updates

Conversation

@stephanie-owen
Copy link

Plotting code modifications. All mods added to each individual script in ecodata (plot_xxx.R) can be seen in the commented out lines in this script (https://github.com/NEFSC/READ-EDAB-SOE_reports/blob/stephanie_dev/scripts/create_plots.R).

All updated functions in the 'plot_updates' branch have been tested in this script (https://github.com/NEFSC/READ-EDAB-SOE_reports/blob/stephanie_dev/scripts/create_plots_new_SO.R) to ensure they produce the correct figures.

.groups="drop")
dplyr::summarise(Value = sum(Value), .groups = "drop") |>
subset(!is.na(Value)) |>
dplyr::mutate(Fishery = dplyr::recode(Fishery,
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be doing this. If we need to recode variables we should be doing it programmatically

@andybeet
Copy link
Member

Not sure i understand why this was done!

@atyrell3
Copy link
Contributor

There have been a lot of post-hoc figure adjustments included in the report knitting in the past. As part of the updated Rmarkdown process and to improve the figure versioning, we're moving all the plotting code into the ecodata functions.


if (report == "MidAtlantic") {
p <- p +
ggplot2::coord_cartesian(ylim = c(2e+07, 4e+07), xlim = c(1998, 2023)) +
Copy link
Member

Choose a reason for hiding this comment

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

cant hard code years, what happens next year?

@andybeet
Copy link
Member

Far too many issues, mostly hardcoded times and names. The PR description should say explicitly why this is needed. There are several products that rely on ecodata, the catalog in particular. Making these types of changes could cause big problems

#'

plot_seal_pups <- function(shadedRegion = NULL,
report="MidAtlantic") {
Copy link
Member

Choose a reason for hiding this comment

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

The idea was to keep the default report name the same across all indicators

@BBeltz1
Copy link
Collaborator

BBeltz1 commented Nov 19, 2025

Yes, Andy is correct. Let's take some extra time to go through/discuss these things. At first glance, there is a lot of hard-coded formatting being carried over from the report .Rmds to the plot functions... this would work to generate plots for the reports, but will negatively impact several of our products that use ecodata, not to mention impacts to our many end users.

Generally, tweaks to plots specifically for the reports have to live somewhere specific to the reports. Not in the package itself.

report="MidAtlantic") {
report="MidAtlantic",
data = all_data,
port_list = NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

this will break catalog

report="MidAtlantic",
varName = "adult",
n = 0) {
n = 10) {
Copy link
Member

Choose a reason for hiding this comment

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

why has default changed?

@BBeltz1 BBeltz1 requested a review from andybeet November 19, 2025 18:10
@atyrell3
Copy link
Contributor

Let's circle back to this in the spring/summer. Thanks!

@atyrell3 atyrell3 marked this pull request as draft November 19, 2025 20:05
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.

5 participants