Coverage rate dropped from 95% to 91%

[sigmafelix]

I found that the coverage rate has dropped to 91% after two consecutive merging that were made six minutes apart. Our ubuntu (devel) runner usually takes 30+ minutes to finish R CMD CHECK, so I would recommend waiting for 40 minutes after a merge is initiated at main. Merging main into your branch while another merge is running will result in this type of silent conflict/rollback. We might try using some configurations as described in a StackOverflow discussion to avoid similar instances in the future.

[mitchellmanware]

I noticed that as well. Agreed that we should wait in between pull requests, but i can also write a few unit tests for the support functions to ensure all lines are covered, as well as replace path defaults as discussed in https://github.com/Spatiotemporal-Exposures-and-Toxicology/amadeus/issues/7#issuecomment-1964720307.

@sigmafelix @eva0marques Should the default values simply be NULL and rely on the check_for_null_parameters(mget(ls())) to catch empty parameters, or should we re-create defaults based on new folder structure. My vote is to use NULLs for simplicity and consistency, but I am open to either.

[sigmafelix]

It seems like several hundred lines of test files were reverted in the current main. I'm trying to resolve the issue in git way, but I will end up copying and pasting (old-fashioned 😅) the reverted lines back if this try is not as efficient as it is supposed to be.

@mitchellmanware I agree on replacing path default to NULL. I think other default arguments that would be rarely changed by users have their own good for usability and user convenience (i.e., reducing some keystrokes).

[kyle-messier]

@sigmafelix @mitchellmanware @eva0marques We should discuss how code and/or tests were removed, but passed the tests and review process of the repo.

[mitchellmanware]

I will replace with NULL. Follow up if I can help restore the tests.

[mitchellmanware]

@Spatiotemporal-Exposures-and-Toxicology @sigmafelix @eva0marques I believe the MODIS unit tests were disable by skip() in Eva's branch which was merged into the main. Although she pulled my updated tests into her branch, the skipping of these tests reduced the code coverage https://github.com/Spatiotemporal-Exposures-and-Toxicology/amadeus/pull/18/files.

I had approved her file changes before she pulled the main into her branch, so my pre-merge approval still allowed her to pull her branch into the main without the tests passing. I should have communicated better what I was approving with my review.

[mitchellmanware]

I removed skip() from unit tests. Tracking on branch mm-unit-tests (https://github.com/Spatiotemporal-Exposures-and-Toxicology/amadeus/pull/23) to see impact on code coverage.

[sigmafelix]

@Spatiotemporal-Exposures-and-Toxicology @mitchellmanware @eva0marques My fault. I've been confused with the commit before the tests were added and the current main. I'm sorry to make y'all confused.

[eva0marques]

I passed all the tests after @mitchellmanware review. I actually passed them even before your pull request. But in both cases I had a code coverage error because as Mitchell said I skipped the download MODIS tests. The testcoverage check decrease was only due to this file so for me none of the tests are removed, just MODIS one is disabled 🤔

[eva0marques]

@mitchellmanware I am lost about your question on NULL parameters, can you explain again? 😬

[mitchellmanware]

The default setting for directory_to_download = and directory_to_save = in the download functions and for path = in the process functions is based on the folder structure we adopted for the beethoven project.

download_modis_data <- function(
    date_start = "2023-09-01",
    date_end = "2023-09-01",
    product = c(
      "MOD09GA", "MOD11A1", "MOD06_L2",
      "MCD19A2", "MOD13A2", "VNP46A2"
    ),
    version = "61",
    horizontal_tiles = c(7, 13),
    vertical_tiles = c(3, 6),
    nasa_earth_data_token = NULL,
    directory_to_save = "./input/modis/raw/",
    data_download_acknowledgement = FALSE,
    mod06_links = NULL,
    download = TRUE,
    remove_command = FALSE) {

We are suggesting to replace these defaults with NULL as it is very unlikely users will want to use this same file structure, and running the function with default directories/paths may result in the data being downloaded to a random folder the user cannot access or does not have. Each download function contains this code check_for_null_parameters(mget(ls())), which returns an error if any of the parameters are NULL, so the default setting of NULL will force the users to set the saving directories themselves.

[eva0marques]

I got you, and I agree :)

[mitchellmanware]

User will see an error like this if they do not set a directory.

download_hms_data(
+   data_download_acknowledgement = TRUE,
+   download = TRUE
+ )
Error in download_hms_data(data_download_acknowledgement = TRUE, download = TRUE) : 
  argument "directory_to_download" is missing, with no default

[mitchellmanware]

All checks pass on https://github.com/Spatiotemporal-Exposures-and-Toxicology/amadeus/pull/23 and code coverage has increased. I will create the NULL defaults and additional unit tests on a new branch.