Suggestion of simplifcation for the vignettes and article
Closed this issue · 3 comments
As a user, as a beginner, as a person who do not known IceSat let me suggest you what kind of content I'd like to see in the vignette 1. This suggestion is related to my review where I said that the documentation and vignettes are tedious to read and overly complex. I suggest to create higher level functions and respect common data storage practice such as the vignette 1 looks like that.
require(IceSat2R)
require(sf)
sf::sf_use_s2(use_s2 = FALSE)
data(greenl_sh_east)
bb = st_bbox(greenl_sh_east)
rgt_winter = time_specific_orbits("2020-12-15","2021-02-15", bbox = bb, threads = 4, verbose = TRUE)
rgt_summer = time_specific_orbits("2021-06-15","2021-08-15", bbox = bb, threads = 4, verbose = TRUE)
7 lines and we are already somewhere between line 35 and 45.
Then there is the "unique RGT" stuff. You may keep it as is or include a function in your package to do it. It looks standard and repetitive so it should go in the package in my opinion. No need to print stuff. Beginner are able to use length(unq_rgt_winter)
themselves. 2 lines instead of 15
unq_rgt_winter = clean_duplicates(rgt_subs_winter)
unq_rgt_summer = clean_duplicates(rgt_subs_summer)
Then we have the definitionof verify_RGTs
. This should be a function of the package. We gain another 40 lines of code.
Then we have twice a complex set of code around verify_RGTs
. It becomes 2 lines of code because verify_RGTs
already set the correct column names and is internally able to deal with standard data.
ver_trc_winter = verify_RGTs(rgt_winter)
ver_trc_summer = verify_RGTs(rgt_summer)
Then we have
greenl_grid = degrees_to_global_grid(greenl_sh_east, degrees = 4.5, verbose = TRUE)
And I stopped here. The first half of the vignette 1 can be contained in 12 lines with a careful design. Nobody want to go into lines and lines and lines of tedious code in the first learning step 😉
Thanks for reminding me the high level functions. I'll implement these in the next days and I'll notify you so that you can review the changes.
in the latest commit,
- I added the ne_10m_glaciated_areas.rda and RGT_cycle_14.rda files in the data directory and I removed the ne_10m_glaciated_areas.RDS and RGT_cycle_14.RDS files from the inst directory
- I've added the verify_RGTs() function (in the API_utils.R file)
- I modified / updated the Vignettes, Example sections and the JOSS paper taking into account all previous mentioned changes
I mentioned all changes in the NEWS.md file. From the JOSS paper I removed the code related to the verification of the RGTs and I also simplified the last for-loop (a double for-loop is not required to show the functionality of the package). I also removed R package dependencies that are not used in the code.
I went through the code of the package once again and I performed the necessary changes. The vignettes (especially the "IceSat-2_Atlas_products.Rmd" file) include details and use cases. Although, it not required for an author of an R package to include a use case (or too many details) I'm used to explain the functionality of my R packages that way. Moreover, I expect that R users of my R packages have R programming rudimentary skills.
I explained the reason why a high level is not wise in the Joss review.
Potential R users of this R package are free to open a pull request in case such a high level function can be beneficial for their work or scientific study.
I close this issue for now. Feel free to re-open it in case the code does not work as expected.