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

Formula mapping #43

Merged
merged 19 commits into from
Dec 13, 2024
Merged

Formula mapping #43

merged 19 commits into from
Dec 13, 2024

Conversation

ericward-noaa
Copy link
Collaborator

What was changed/addressed

  1. I fixed 2 outstanding doc issues that were causing warnings with devtools::check()
  2. For the yellowtail examples to be able to run, we need to pass in the sdmTMB control list, so added that as an argument
  3. I also added share_range as an argument -- this is fixed to FALSE internally, but the default in sdmTMB is TRUE -- I think we want it to be TRUE for more data limited species
  4. Added configfuration script that demonstrates how to fit the model with yellowtail

@chantelwetzel-noaa
Copy link
Collaborator

@ericward-noaa Thank for putting this together and including the code you used for yellowtail. Thinking towards when I am working on assessment prioritization where I would like to reproduce the yellowtail index using the same model configuration and when I am estimating indices for a wide-range of species, many data limited, should we be adding a column in configuration csv for share_range? Are there other changes to the configuration csv that we should also be considering?

@ericward-noaa
Copy link
Collaborator Author

Yes -- I think adding a column for share_range would be great. No need to change it for the existing species in there, but would give some flexibility in the future

@chantelwetzel-noaa
Copy link
Collaborator

I am testing the code in this pull request now and will have feedback when everything runs and/or suggested changes.

@chantelwetzel-noaa
Copy link
Collaborator

@ericward-noaa I have tried running the yellowtail configuration code twice and am getting the following error:

ℹ Fixing or mirroring b_j
ℹ Fixing or mirroring b_j2
Error in dplyr::mutate():
ℹ In argument: results = purrr::pmap(...).
Caused by error in purrr::pmap():
ℹ In index: 1.
Caused by error in purrr::map() at indexwc/R/calc_index_areas.R:99:3:
ℹ In index: 1.
ℹ With name: Coastwide.
Caused by error:
! object 'region' not found

I see the region column in the data$data_filtered[[1]] object. It seems to be happening after or late during fitting where the only two files being written are the hess_logical and ignore_fit.rds files. Any thoughts on why I may be getting this error?

@ericward-noaa
Copy link
Collaborator Author

Yeah, I think I know what's going on. The changes work fine for the original configuration script -- but with the yellowtail example, the 'region' variable is included in the estimation model and not in the prediction grid. I think it's probably up to you or @okenk / @iantaylor-NOAA on how best to deal with this in a generalizable way (so it'd be easy to adopt the same models for similar species with splits in the future). The boundaries_data object is in there, but I haven't exactly figured out how the related function calls are working. If there are region IDs in the prediction grid already, the easiest thing to do would be to pass those into the formula, rather than the 'region' variable I created

@ericward-noaa
Copy link
Collaborator Author

Related to the above question on the extra column for share_range, I think we also could use an extra column(s) describing the latitude breaks for indices generated in the domain. Right now, the max / min latitude and longitude columns are used for a priori filtering. Yellowtail is an example of a species we'd want to generate indices for multiple sub-regions (N/S of Pt Conception). The easiest thing to do would be one per row -- so the "yellowtail rockfish" row would correspond to "yellowtail north" -- and the extra columns would be something like index_lat_min / index_lat_max / index_lon_min / index_lon_max to denote the region of the prediction grid used for making the index.

@chantelwetzel-noaa
Copy link
Collaborator

I have modified the configuration.csv adding a share_range column and have made modifications to the yellowtail row to match the configuration in the yellowtail example but have not pushed since I did not want to mess anything up without confirming that everything runs correctly.

I think you are correct on why my run is throwing an error. I will work with @kellijohnson-NOAA to see if I can modify the grid to include or add a column when necessary depending upon the model configuration.

@chantelwetzel-noaa
Copy link
Collaborator

@iantaylor-NOAA I have worked through these changes and am happy with all the changes. Would you like to review this prior to me merging this pull request?

Copy link
Collaborator

@iantaylor-NOAA iantaylor-NOAA left a comment

Choose a reason for hiding this comment

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

I just did a really superficial review as I don't have time to dig into the detail configuration stuff right now and trust that both @chantelwetzel-noaa and @ericward-noaa know what they are doing.

R/run_sdmtmb.R Outdated Show resolved Hide resolved
data-raw/configuration.R Show resolved Hide resolved
data-raw/grid_nwfscSurvey.R Outdated Show resolved Hide resolved
@iantaylor-NOAA
Copy link
Collaborator

I just pushed two minor additions to this pull request

  • a second occurrence of 46.0 that should be 46.25 (should have trivial impact on results but better to have it correct).
  • increment the package version from 0.6 to 0.7

@chantelwetzel-noaa, you're welcome to merge any time as far as I'm concerned.

@chantelwetzel-noaa chantelwetzel-noaa merged commit 8f380fe into main Dec 13, 2024
10 checks passed
@chantelwetzel-noaa chantelwetzel-noaa deleted the formula-mapping branch December 13, 2024 20:21
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