Conversation
Schiano-NOAA
left a comment
Schiano-NOAA
left a comment
There was a problem hiding this comment.
@kellijohnson-NOAA I really like these changes you have incorporated. I greatly appreciate your suggestions to streamline this. I will definitely be implementing these concepts in the code for the other figures and probably tables. I am not merging this right now since it includes the hake data which is definitely fairly large as you mentioned in the other PR (unless this is a shortened version?) However, if this is set to merge, then I say go for it. Later, I can work on getting a better example file that's smaller. I am thinking using simulated data from Bai's project maybe or a very simple SS3 example.
I will also look over the issues you opened, consider your streamlined approach, and begin to develop a more set plan that will allow me to delegate tasks among you, me, Sophie, and Meg (once she's back), if that works for you!
|
I can easily pull out the commit with the data and force push the relevant commits back up. If it is okay, I would like to work on this for one more day to double check some things. |
kellijohnson-NOAA
force-pushed
the
streamline-plots
branch
from
e3e3b22 to
cc83d44
Compare
|
I redid the PR with a simplified version of plot_spawning_biomass. I tested the figure with two data sets provided by @Schiano-NOAA but there are no examples in the code because there is not a data set within the package to test it with. I could write a test for bogus data if you want. Please see the commit message for items in the PR. |
|
Leaving a comment because both conflicts and a couple small changes to the code got pushed together: Fixed some conflicts in the branch and removed the read.csv in favor of output = dat to improve performance for future tasks such as rendering the report per the conversation with @kellijohnson-NOAA. Let me know if you think it should be reverted back to read.csv. Documentation must be updated in response. |
|
@Schiano-NOAA I will rebase to master and then force push again. We want to get rid of the four initial commits where I was messing with the data object (i.e., the other pull request). I can change dat to be a read in object though if you want. |
|
That sounds good! I think it's probably better to read in as an object rather than the csv. |
|
So, I am now modifying all the functions to work with a dat object rather than reading it in but I am unsure if I should delete all of the code that looks for what type of model and then does something different for BAM and WHAM? Do you want to save that for a different time or should I do it now? |
|
Do it now since you're there anyway. I was planning on doing it after we had figured out how we handle spawning biomass (using it as a model) then apply it to the rest. |
Just have a standard plot and not all the if else statements by using
simple ifelse statements in the code. Also, removed plots for non-standard
output per Sam's guidance.
Changes to all plotting and tables
* dat is an object rather than reading in the file every time a figure or table
is created, this change is carried over throughout the documentation and the
code in all functions.
* Removed SS3 and BAM methods
Changes to `plot_spawning_biomass()`
* There was a problem with `if(any(grepl("target", output$label)))` because it
ignored what the user specified in `ref_line` if target is present in label,
i.e., you could never get to MSY. There should be a two-level check, i.e.,
what does the user want and is it present. I have not fixed this latter
problem.
* Fixed use of `..., ]$estimate` to `..., "estimate"]` because you should not
use the dollar sign which allows for partial matches.
* Use `switch()` statements instead of nested `if` `else` statements. Actually,
now the function is so simple that I do not even have any switch statements
but they can be helpful instead of nested if else statements.
* Fixed the non-relative plots to truncate data to only use years less than end_yr
* Some of the scaled items were hard-coded to 1000 instead of using scale_amount
* Removed show_warnings because it was not actually doing anything
* Use end_year rather than renaming it to endyr
* Subset sb to end year before determining the correct number of breaks
* MSY does not work so I removed it from the default reference list, only
target and unfished work
Need to fix:
* units are not divided by scale amount to create new unit label
* Why do we think that our determination of breaks is better than what ggplot2
provides?
* There is no example
kellijohnson-NOAA
force-pushed
the
streamline-plots
branch
from
894f2b3 to
6664ebd
Compare
|
The recent code removes the model argument and changes dat to a read in object rather than a file. I think that this is good to go but you should probably give it a glance or let me know how I can check it with asar before it is merged. |
|
Thanks Kelli! I will give it a test on a couple example files then merge this in. As for working with asar, the current connection between the 2 tools is that there are pre-built chunks that contain the satf function already written. |
kellijohnson-NOAA
force-pushed
the
streamline-plots
branch
from
925cbd5 to
6664ebd
Compare
3 participants
@Schiano-NOAA this is just a draft PR to show you what I worked on over the weekend. I think that there is a lot of redundant code in the plotting functions that can be worked on. I think the function would be more readable with
switch()statements instead of nestedifelsestatements.Also, I found the following issues.
if(any(grepl("target", output$label)))because it ignored what the user specified inref_lineif target is present in label, i.e., you could never get to MSY. There should be a two-level check, i.e., what does the user want and is it present. I have not fixed this latter problem...., ]$estimateto..., "estimate"]because you should not use the dollar sign which allows for partial matches.The following still needs to be fixed:
I am not sure where you want to go with this. I made some major changes without asking and I wanted to make sure that they are inline with your development plan. If they are not, then maybe we can meet so we can talk about how I can be helpful instead of just changing code that I am not supposed to be changing 😉. Anyway, I wanted to check in before I did anything more.