-
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
Add indicator native range year #65
Conversation
#64 this should enable setting the axis labels in english & dutch (default english)
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.
Thanks @SanderDevisscher for adding this function to trias package!! 👍 I like you added your experience in making interactive plots. If this draft is completed, I could consider to convert other visualize functions as well. I wrote a detailed review to help you in the fantastic world of package development. Just let me know if something is not clear. I like to learn these stuff myself few years ago, I hope you too.
Important: typically the filename is the same as the R function it contains. I would consider to rename the function to ìndicator_native_range_year
. In this way all visualization functions are called ìndicator_*.R
.
R/indicator_native_range_year.R
Outdated
@@ -0,0 +1,117 @@ | |||
#' Create interactive plot for counts per native region and year of introduction | |||
#' | |||
#' Based on \code{\link{countYearProvince}} plot from grofwild |
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 like you use links for more information about code in documentation 👍
But if I compile the documentation via devtools::document()
I get:
> devtools::document()
Updating trias documentation
Loading trias
Writing NAMESPACE
Writing NAMESPACE
Writing countYearNativerange.Rd
Warning message:
Can't find help topic 'countYearProvince' in current package
This means the link is broken. R thinks your link is in trias package. Is grofwild
a package? Then you can use something like this \code{\link[MASS]{abbey}}
? But I don't think it is a package. Then maybe a classic Markdown-style link solves the problem, e.g.:
#' See more about the markdown markup at the
#' [Commonmark web site](http://commonmark.org/help)
This vignette is very helpful: https://cran.r-project.org/web/packages/roxygen2/vignettes/rd-formatting.html#links
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 like you use links for more information about code in documentation 👍
I just copied that part from @eadriaensen => should I include her as contributor as well ?
Is
grofwild
a package?
reportingGrofwild is a package, yes! However it is not published to cran or any other package repository.
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.
@damianooldoni the link to the reporting-grofwild function has been rewriten but I don't know how to check if its working.
The installation of the modified trias package runs without new errors (expect the error described here #65 (comment)).
data_input_checklist_indicators <- read_delim("https://raw.githubusercontent.com/trias-project/indicators/master/data/interim/data_input_checklist_indicators.tsv", | ||
"\t", escape_double = FALSE, trim_ws = TRUE) | ||
|
||
source("./R/indicator_native_range_year.r") |
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.
You don't need and don't have to source within a package. Installing a package and loading it makes all functions available for tests as well.
Run devtools::install()
and ´library(trias)(the latter command needed the first time) after you changed something in the functions and you have all functions ready to be used. So, in package development,
devtools::install()is your best friend,
source` your enemy 😄
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.
When I use devtools::install() with the script without plotly::layout() i get this error:
Note: possible error in 'layout(xaxis = list(title = x_lab, ': unused arguments (xaxis = list(title = x_lab, tickangle = "auto"), yaxis = list(title = y_lab, tickformat = ",d"), margin = list(b = 80, t = 100), barmode = ifelse(nlevels(summaryData$first_observed) == 1, "group", "stack")) at indicator_native_range_year.R:104
I suspected this was due to R thinking layout from the package graphics should be used, while the importFrom line clearly states layout should come from the plotly package (@importForm plotly ggplotly, layout
).
Adding plotly::
before layout solves this problem. Any idea why this is ?
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.
somehow rebuilding the NAMESPACE (found out some functions I imported were missing) does not include plotly to the list. Maybe this is the cause??
|
||
source("./R/indicator_native_range_year.r") | ||
|
||
countYearNativerange(data_input_checklist_indicators, jaartallen = c(1990:2019), type = "native_range", relative = FALSE) |
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.
This is not a test. Testing plotly output is practically impossible or quite challenging, but you can test the data slot, and the fact that you get always a list and not something else... Writing tests is something which takes time, I know, but it helps making your code more robust and user friendly. For example, testing the input types, See other tests functions in trias package to understand what I mean. But I don't blame you if you don't want to spend time on the tests, I will do it.
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.
This makes this function more flexible and more similar to other functions of the package.
It seems they are never used in this function. @SanderDevisscher : I remove them. Any objections?
Imported function
( ) instead of [ ]
@SanderDevisscher: I improved documentation (typo of some arguments or not documented) and I also added argument About the rest:
Ready to merge. @SanderDevisscher: could you please double check that the function you need is still working as you wish? |
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
==========================================
- Coverage 94.67% 92.71% -1.97%
==========================================
Files 15 16 +1
Lines 2442 2526 +84
==========================================
+ Hits 2312 2342 +30
- Misses 130 184 +54
Continue to review full report at Codecov.
|
@damianooldoni I tested the function and added some small improvements to the static_plot (xlab, ylab & x axis text angle). |
Thanks @SanderDevisscher. I review a last time and triple check everything again. I am surely going to merge before the end of today. |
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.
Everything seems ok now.
fix #64