-
Notifications
You must be signed in to change notification settings - Fork 1
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
Proposal of some updates to read_habitatmap_xxx() #140
Conversation
This makes it conform 2020, which seems to use a more logical ordering.
Co-authored-by: Cécile Herr <31855012+cecileherr@users.noreply.github.com>
Co-authored-by: Cécile Herr <31855012+cecileherr@users.noreply.github.com>
From https://github.com/inbo/n2khab-preprocessing/blob/b088c44/src/generate_habitatmap_stdized/10_generate_habmap_stdized.Rmd: > An exception to this rule are following codes: `3130,rbbmr`, `3140,rbbmr`, `3150,rbbmr` and `3160,rbbmr`. Also in the R code of above file, it can be seen it is only these codes that are handled.
R/read_habitatdata.R
Outdated
#' created for each of them. | ||
#' The variable \code{certain} in this case will be \code{TRUE} for both types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About issue #117:
you may want to add a note on the interpretation of phab for code combinations from one column of the original habitatmap
Should also be mentioned in the geospatial hab vignette, but I think this can be useful here too, so:
#' created for each of them. | |
#' The variable \code{certain} in this case will be \code{TRUE} for both types. | |
#' created for each of them, with \code{phab} for each new row simply | |
#' set to the original value of \code{phab}. | |
#' The variable \code{certain} in this case will be \code{TRUE} for both types. | |
#' \itemize{ | |
#' \item Note that this implies that the a given polygon could contain the same | |
#' type with variable \code{certain} as \code{TRUE} several times, e.g. when: | |
#' \code{31xx_rbbmr} is present with \code{phab} = yy% and | |
#' \code{31xx} is present with \code{phab} = zz%. | |
#' In that case the rows with the same \code{polygon_id}, \code{type} | |
#' and \code{certain} were gathered into one row and the respective | |
#' \code{phab} were summed up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks! I'll re-add your suggestion as a new commit, as the lines it depends on are flagged as outdated (so cannot apply suggestion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm supposing this bullet is at a lower level. The {} of \itemize needed to be closed 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cecileherr Are you sure this is restricted to 31xx_rbbmr
and it does not occur in general?
Anyway, the code executes this as a general rule. So I think it will be better to state it in general and add this bullet as a subnote under the first bullet (where you added 'with phab for each new row simply set to the original value of phab').
I leave consideration to you; I'll postpone the initial plan of adding the above suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote: it appears that '%' needs to be escaped as \%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cecileherr Are you sure this is restricted to
31xx_rbbmr
and it does not occur in general?Anyway, the code executes this as a general rule. So I think it will be better to state it in general and add this bullet as a subnote under the first bullet (where you added 'with phab for each new row simply set to the original value of phab').
I thought it was a general rule in the code, but that it only happened for the 31xx_rbbmr case, but I was wrong (e.g. 605723_v2014 is an example for the uncertain case). I would be tempted to add this is as 3rd item (after the uncertain case and the 31xx_rbbmr case), instead of making a subitem out of it.
Something like (other items abbreviated):
#' \itemize{
#' \item For some polygons the vegetation type is uncertain (...)
#' if only one vegetation type is provided.
#' \item Some polygons contain both a standing water habitat type (...)
#' created for each of them, with \code{phab} for each new row simply
#' set to the original value of \code{phab}.
#' The variable \code{certain} in this case will be \code{TRUE} for both types.
#' \item After those first two steps, a given polygon could contain the same
#' type with the same value for \code{certain} several times, e.g. when:
#' \code{31xx_rbbmr} is present with \code{phab} = yy\% and
#' \code{31xx} is present with \code{phab} = zz\%.
#' In that case the rows with the same \code{polygon_id}, \code{type}
#' and \code{certain} were gathered into one row and the respective
#' \code{phab} were summed up.
#' \item For some polygons the original vegetation code in the (...)
#' code was adjusted.
#' }
@florisvdh : your thought on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea @cecileherr. It better fits the idea of 'steps'.
Minor comments:
when:
drop colonsummed up
: means 'enumerated'; should become something like 'added up'.with the same value for \code{certain} several times
: I think '[...] repeated several times' is better
I suggest you add another suggestion in this PR, or else do it as commit in your branch update_habitat2020_ch
(including the Rd file update) after merging this PR. Merging this branch is up to you anyway, since its target branch is yours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for the update (and sorry, I had obviously missed issue 117)
Maybe just take a look at this suggestion:
#140 (comment)
and then I think we will have covered all open questions, so we should be able to merge
Thank you for reviewing @cecileherr 🙏. |
Co-authored-by: Cécile Herr <31855012+cecileherr@users.noreply.github.com>
@cecileherr for me this PR is finished, you can merge |
Many thanks ! Let's go! |
filter_hab
to the front: in case of a mismatch the error will come up immediately. Otherwise thehabitatmap
data source is read first, which can take long.read_habitatmap_stdized()
: adding the exception for"3130,rbbmr"
etc. sincehabitatmap_stdized_2020_v1
.Also addressed some remaining parts of issue #117. Specifically I propose for the 2018 versions to reorder the
xxx_types
tibble columns for stdized/terr according to the order of the 2020 datasource version, where this was done in a more logical way. Do you agree? - Also pinging @ToonHub for this one.Commit 9eff6ec on documenting the key goes back to inbo/n2khab-preprocessing#50 (comment):
Results of testing the functions
Created on 2021-05-11 by the reprex package (v2.0.0)
Session info