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.

Sign up for GitHub

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

Jump to bottom

Refactoring #75

Closed
1 of 4 tasks
sigmafelix opened this issue May 1, 2024 · 10 comments
Closed
1 of 4 tasks

Refactoring #75

sigmafelix opened this issue May 1, 2024 · 10 comments
Assignees
@sigmafelix

Comments

@sigmafelix
Copy link
Collaborator

sigmafelix commented May 1, 2024 •
edited
Loading

This is a work list for v1 (CRAN or ROpenSci submission, whatever is first). Here I make a quick to-do list for refactoring process_* and calc_* functions in amadeus.

  • Reduce the number of function calls that read data from storage (e.g., terra::rast(), terra::vect(), terra::time() etc.) by assigning the output of repetitively used functions once earlier then use the object later multiple times
  • Unifying output class of calc_*, plain data.frame or data.table or tibble
    • Data layout: long or wide data frame?
  • (Discussion point) Human-readable field names versus field codes based on a naming convention as used in beethoven
@sigmafelix sigmafelix self-assigned this May 1, 2024
@mitchellmanware
Copy link
Collaborator

@sigmafelix

I can work on point 2 (unifying output class of calc_ functions) while updating for the geom = parameter.

@sigmafelix
Copy link
Collaborator Author

@mitchellmanware Thank you for your help. Please let me know when the update is finished. I will merge your branch into mine before making a PR.

@kyle-messier
Copy link
Collaborator

@mitchellmanware Are you interested in submitting this to ROpenSci? @sigmafelix and I discussed this. He did for chopin and it has been a positive experience thus far. I don't think it would impact the EM&S journal submission but may be worth checking

@kyle-messier kyle-messier reopened this May 1, 2024
@kyle-messier
Copy link
Collaborator

Sorry didn't mean to close

@mitchellmanware
Copy link
Collaborator

If @sigmafelix has found it valuable then I am sure it will be useful for amadeus as well. I am not very familiar with ROpenSci but I'll do some reading on it.

@sigmafelix
Copy link
Collaborator Author

@kyle-messier @mitchellmanware We can consider ROpenSci after publishing amadeus on CRAN. One good aspect of submitting a package to ROpenSci is that it will increase visibility to the larger audience (they post a monthly newsletter that is also posted in R communities). ROpenSci has a little stricter requirements on function examples (they recommend to add examples in all functions), which we need to be aware of. I suggest targeting CRAN first, then discuss ROpenSci later.

@kyle-messier
Copy link
Collaborator

@sigmafelix @mitchellmanware I am thinking we should submit the publication EM&S first as well, then in pararllel can submit to ROpenSci. Given we want this manuscript to be published or at at least submitted by the time we submit the beethoven manuscript.

@mitchellmanware
Copy link
Collaborator

mitchellmanware commented May 3, 2024 •
edited
Loading

So CRAN first, then written version to EM&S, and ROpenSci after those? That seems like a logical progression to me.
In the manuscript I mention how other data oriented packages are not published to CRAN, so having that is important for differentiating amadeus from the previous work.

@kyle-messier @sigmafelix

@sigmafelix
Copy link
Collaborator Author

@kyle-messier @mitchellmanware Agreed on the progression. Let's submit amadeus as soon as v.0.2.0 (I think we can call CRAN-ready version as v.1.0.0, what do you think?) is ready.

@mitchellmanware
Copy link
Collaborator

I think it sounds great. Thanks Insang!

sigmafelix added a commit that referenced this issue May 6, 2024
@sigmafelix
0.2.0dev
4a7f116 
- Significant changes
  - Due to bug fix of process_modis_swath, previous calculations are invalidated and should be rerun
  - Dropped doParallel and foreach dependency: future and future.apply in place
- Bug fix
  - Tests on Date class is removed or replaced with POSIXt
  - HMS: empty/missing polygon error due to date sequence generator setting (i.e., sub_hyphen=TRUE)
  - calc_terraclimate: promise before evaluation error due to introducing geom argument
  - process_modis_swath: empty subdataset processing, clarified documentation on setting subdataset argument
  - process_modis_warp: first-phase mosaicking is done with stars::st_mosaic
  - Most calc_* functions for yearly updated raw data: explicit integer assignment complying to calc_check_time logic
- Refactoring #75
- Etc.
  - Edited calculate_covariates tests
  - Edited calc_gmted for succinct field names
  - Edited process_gmted for the raw data compatibility
  - File name change of nei test data (...17.csv -> ...2017.csv)
  - Styling roxygen2 and elaborating documentation
@sigmafelix sigmafelix mentioned this issue May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sigmafelix sigmafelix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sigmafelix @kyle-messier @mitchellmanware