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

raster helper function edits: attributeList #199

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dvirlar2
Copy link
Contributor

Changed the usage of an attributes data frame to a listed attributeList within the function, and edited the documentation to reflect those changes.

From what I've seen, most PIs input the raster layer in the web editor, so converting the single attributeList to a data frame feels like an odd side-step. If a raster is missing an attributeList, I think the better way forward would be to create an attList either through the webform or Shiny. If done through Shiny, then running set_attributes() and adding it back to the otherEntity before converting to a spatialRaster, rather than reverting an attList to a data frame.

Changed the usage of an attributes data frame to a listed attributeList within the function, and edited the documentation to reflect those changes.
Added a warning message if the coord_name argument is NULL. Warning message alerts users that EML won't validate, and provides correct string for commonly used WGS84 and NAD83 datums. Also directs users to the get_coord_list() for more information.
@dvirlar2
Copy link
Contributor Author

raster helper function edits: coord_name argument

Added a warning message if the coord_name argument is NULL. Warning message alerts users that EML won't validate, and provides correct string for commonly used WGS84 and NAD83 datums. Also directs users to the get_coord_list() for more information.

@dvirlar2 dvirlar2 marked this pull request as draft October 20, 2022 00:30
@dvirlar2 dvirlar2 marked this pull request as ready for review October 20, 2022 00:45
@dvirlar2
Copy link
Contributor Author

No longer a draft 👍🏽

Copy link
Collaborator

@jeanetteclark jeanetteclark left a comment

Choose a reason for hiding this comment

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

this looks pretty good - are all of the tests for the package passing?


if (is.null(coord_name)){
coord_name <- raster::crs(raster_obj)@projargs
#coord_name <- raster::crs(raster_obj)@projargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove this commented out line entirely

@dvirlar2
Copy link
Contributor Author

I haven't ran any formal tests within the package's test folder, but I was running tests between each push to make sure things were working and error-ing in the right/expected places. I can add a test to the end of the test_eml.R file, I'll just need a quick overview of how that works. I'll read through the testing chapter in the R Packages manual later today

@jeanetteclark
Copy link
Collaborator

Yeah formalizing what you did ad-hoc as a test, in addition to running the rest of the tests (via devtools::test()) would be good. I should have written a test when I added this function initally

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.

2 participants