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

Add indicator native range year #65

Merged
merged 63 commits into from
Sep 16, 2020

Conversation

SanderDevisscher
Copy link
Contributor

fix #64

Copy link
Collaborator

@damianooldoni damianooldoni left a 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 Show resolved Hide resolved
R/indicator_native_range_year.R Outdated Show resolved Hide resolved
R/indicator_native_range_year.R Outdated Show resolved Hide resolved
@@ -0,0 +1,117 @@
#' Create interactive plot for counts per native region and year of introduction
#'
#' Based on \code{\link{countYearProvince}} plot from grofwild
Copy link
Collaborator

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

Copy link
Contributor Author

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.
image

Copy link
Contributor Author

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)).

R/indicator_native_range_year.R Outdated Show resolved Hide resolved
R/indicator_native_range_year.R Outdated Show resolved Hide resolved
R/indicator_native_range_year.R Outdated Show resolved Hide resolved
R/indicator_native_range_year.R Outdated Show resolved Hide resolved
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")
Copy link
Collaborator

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 😄

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

image

all jokes aside, I simply used this to test if the function worked.

@damianooldoni
Copy link
Collaborator

@SanderDevisscher: I improved documentation (typo of some arguments or not documented) and I also added argument first_observedwith default value "first_observed" as the name of the column containing year of introduction. This makes the use of this function similar to other ones and also more flexible in case the name of the column change in the future.

About the rest:

  1. Package is compatible with rgbif 3.x and (>= 3.0) added in DESCRIPTION
  2. Problem mentioned above (Add indicator native range year #65 (comment)) with ordering of output taxa in function verify_taxa() caused by dplyr 1.0 solved
  3. I also wrote test to check that taxon keys are unique in input taxa and verification file, both inputs of verify_taxa(). And also that no missing values are present (= rows without taxonKey are not allowed).
  4. Other minor stuff

Ready to merge. @SanderDevisscher: could you please double check that the function you need is still working as you wish?

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2020

Codecov Report

Merging #65 into master will decrease coverage by 1.96%.
The diff coverage is 49.53%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
R/indicator_introduction_year.R 100.00% <ø> (ø)
R/indicator_native_range_year.R 0.00% <0.00%> (ø)
R/visualize_pathways_level2.R 93.22% <ø> (ø)
R/visualize_pathways_year_level2.R 95.38% <ø> (ø)
R/gbif_get_taxa.R 93.33% <100.00%> (ø)
R/gbif_has_distribution.R 98.07% <100.00%> (ø)
R/gbif_verify_keys.R 91.30% <100.00%> (ø)
R/get_table_pathways.R 100.00% <100.00%> (ø)
R/indicator_total_year.R 100.00% <100.00%> (ø)
R/verify_taxa.R 97.50% <100.00%> (+0.13%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e758217...43a5c84. Read the comment docs.

@SanderDevisscher
Copy link
Contributor Author

@damianooldoni I tested the function and added some small improvements to the static_plot (xlab, ylab & x axis text angle).
For me the function is ready now.

@damianooldoni
Copy link
Collaborator

Thanks @SanderDevisscher. I review a last time and triple check everything again. I am surely going to merge before the end of today.
I have just found that rgbif was installed by remotes (from GitHub) but this was useful at the beginning of TrIAS when we were constantly improving rgbif so we needed to install the most updated version of the master instead of installing the CRAN version. But now it is not so important anymore. So I removed it from DESCRIPTION.

Copy link
Collaborator

@damianooldoni damianooldoni left a 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.

@damianooldoni damianooldoni self-requested a review September 16, 2020 08:49
@damianooldoni damianooldoni merged commit ce5832e into master Sep 16, 2020
@damianooldoni damianooldoni deleted the add_indicator_native_range_year branch March 1, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

add_indicator_native_range_year function
3 participants