Create a function download_models to download ss3 test models #721
Conversation
This test would fail if the super_test feature branch is removed from the test-models repo, so it seems wise to not include it. However, it was a nice local test to make sure the function worked as I expected when setting it up!
Codecov Report
@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 54.88% 51.93% -2.96%
==========================================
Files 111 111
Lines 25975 25985 +10
==========================================
- Hits 14257 13495 -762
- Misses 11718 12490 +772
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@k-doering-NOAA thanks for writing this function. It works great. I tested it out on my local machine but I did not run the tests. GitHub actions is enough for me there 😉
Feel free to ignore all of the nit picky comments. But, there is one substantial design-choice that I am not sure that I agree with. Instead of just stopping if dir doesn't exist cannot we create dir recursively at the start of the function?
|
Thanks @kellijohnson-NOAA for the comments! I'll go over them later today or early next week. I was just reading the ropen sci blog post and wanted to drop a link on this thread; when I was developing the testthat tests for |
suggestions from @kellijohnson-NOAA in PR #721
including running devtools::spell_check() devtools::document() and styler::style_file() for the files related to download_models
|
@k-doering-NOAA sorry for this late comment but did you look at {usethis} for working with zip files, see here? |
|
Aww, I didn't know about this! I actually looked at usethis, because there is a function that can download a file from github, but I didn't know about usethis::use_zip ! Do you think I should modify the PR? The plus of what I wrote is that we don't need to add usethis as a dependency to r4ss, but I can change the code if you would prefer! |
|
I think @iantaylor-NOAA should decide if we want another dependency or not. But, I like that usethis in theory provides the checks rather than us having to write them and it ensures that the correct files are closed. Leaving the code is fine. The benefit of using {usethis} is that it is easier for people to copy the single function into their own code and use it or for us to reuse it later. Not sure if that is going to be a thing here though. |
|
I think making use of the {usethis} function makes sense if it's not too much work to revise all the work that @k-doering-NOAA already put into this. There's nothing about downloading zip files that will ever be unique to {r4ss}, so I'm happy to avoid maintaining custom code when we can share what others are doing and focus on the unique parts of pointing to the model files that we want to download. I've also come to see a difference between depending on less common packages with questionable maintenance (like {gtools} as discussed in ss3sim/ss3sim#383) vs those that have the full weight of the paid RStudio (now Posit: https://posit.co/) staff maintaining them. The {usethis} package seems genuinely useful so nudging {r4ss} users who don't yet have it installed to do so might improve their lives in other ways. |
|
I looked a little more into |
|
Thanks for checking @k-doering-NOAA into {usethis}. I think that instead of suppressing the warning we could ask {usethis} in the future to remove the interactive nature of the function if we ever did want to change. But, I agree that the code that you wrote is a better solution for now given the other one-off file management that must occur. Sorry for the 🐰 hole. |
|
Good idea re: a feature request! Also, the function looks useful for instructing folks on how to download a github repo without needing to clone, so it was definitely worth the time looking into it! |
Resolves #574 by creating a
download_models()function that will download the models subfolder from the nmfs-stock-synthesis/test-models repository. There is an option to specify which branch to download and which directory to write to.I requested @kellijohnson-NOAA to review as I think this was her idea, but feel free to reassign to @iantaylor-NOAA instead if you are too busy.