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

Fixes to plot_cpue #171

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fixes to plot_cpue #171

wants to merge 3 commits into from

Conversation

mcgoodman
Copy link

Fixes the following small errors in plot_cpue.

  • plot argument ignored
  • plot 1 (marginal log(cpue) by depth, latitude) not printed to console
  • Error when catch passed as unnamed (first) argument

Also allows for additional arguments to ggsave (other than height and width), e.g. dpi, scaling, device.

@chantelwetzel-noaa
Copy link
Contributor

@mcgoodman Thank you for creating this pull request. It is difficult to determine exactly what has been changed because it looks like the existing code may have been cut and pasted into if statements. Many of the plotting functions have width and height arguments, so I think it is reasonable that that functionality be available here as well. However, the way it is handled in this pull request is inconsistent with other functions with this functionality.

Here are my initial comments, questions, and requests:

  1. Adding the plotting into if statements based on the requested plots in the function call, is good fix, since it should have been doing this all along.
  2. Prior to bringing in this commit, I would like to request that you make revisions to match how this is handled in other functions. If there are particular reasons why you don't think that would work here, or if you want additional control, please provide clarification on that.
  3. Can you clarify what the final bullet is referring to (Error when catch passed as unnamed (first) argument)?

@mcgoodman
Copy link
Author

Hi Chantel!

Apologies for the copy and pasting, I should have added the if() statements around the existing code blocks. To clarify, the actual plot code is unchanged.

To clarify, for (2), the function will still accept width and height arguments via ..., since it should take any argument to ggsave, but I see what you mean about consistency with other plotting functions - I can add those back as explicit arguments if that's preferable. The defaults are the same.

The last bullet point was just meant to say that I reordered the arguments so that plot_cpue(catch) works (previously plot_cpue(catch = catch) was needed) - catch is the only argument without a default, and the data being plotted is the first argument in most of the other plot functions, so I figured it made sense.

@iantaylor-NOAA
Copy link
Contributor

It's strange that the github view of the file changes is coloring all the indented code. I thought it used to just highlight the whitespace changes brighter with the rest of the line not as bright.

I do see that there's an option in the "files changed" view to "hide whitespace" which ignores those changes entirely which makes it easier to review this PR:
image

@chantelwetzel-noaa
Copy link
Contributor

@iantaylor-NOAA Thanks for the tip! That is super useful.

@mcgoodman
Copy link
Author

Added back in the height and width arguments! Syntax should not be consistent with the other plot functions.

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.

3 participants