A novice user who does not know much about eReefs will be lost about the types of data that are out there.
It would be nice to create a function which scrapes exisiting metadata from netCDF files, and provides a table to users with things like:

• spatial coverage and level of aggregation
• temporal coverage and level of aggregation
• variables and their string names

The above should help people navigate the nuances of the multiple products.

Tentative name suggestions for such function: navigate_ereeefs_data or interrogate_ereeefs_data

Most functions have similar code at the start to set things up (start and end dates, file names and locations) and then loop through the required time-steps and files, month by month or day by day. Each function has its main code inside these loops. We should be able to simplify this by having a function to do all the set-up and call a specified function passed in by name inside the loop. The R package, purrr should be useful here.

Action workflow for R CMD CHECK might need the addition of some tests

Action workflow for pkgdown should only be done after issues #14 #11 are done. Should it work with master only, or perhaps dev as well?

The tidync package is more elegant and intuitive for reading netcdf into data frames, with less chance of errors, but would require substantial code refactoring.

Many functions end with the function return but this is not needed in R for most cases.
return() in R is only useful if an output is to me returned mid-function (e.g. in case something fails and a NULL is to be returned).

• Ideally we should add the code that creates R/sysdata.rda for transparency sake, see example of code and folder structure in
  https://github.com/open-AIMS/bayesnec/blob/master/data-raw/sysdata.R

• Also, I note that some things seem to be for internal use only? gbr*_ , but others might be useful to the users? e.g. map.df. I would differentiate those into separate files and make datasets available as objects, with their own documentation. For example here, https://github.com/open-AIMS/bayesnec/blob/master/data-raw/sysdata.R, you'll notice that the usethis:: call at the end has the argument internal = TRUE. So, this is only visible to us internally. However, other datasets like this one
  https://github.com/open-AIMS/bayesnec/blob/master/data-raw/herbicide_data.R are supposed to be seen by the user, so we don't set it to internal only (and hence do not save it as part of R/sysdata.rda).

• Then you can also add a doco file in the R folder to explain what the data.frame is about:
  https://github.com/open-AIMS/bayesnec/blob/master/R/data.R

• I would also rename data objects to replace . for _

Tibbles and hyper-tibbles are computationally more efficient and easier to display.

This should cover most common usage of the package. Starting from scripts that Barbara created.

This should be the case for multiple functions, such as

map_ereefs
map_ereefs_movie

Workaround: use robust=TRUE
A recent bug fix to get it working for a single profile broke functionality for get_ereefs_profile when applied to a time range.

e.g. Land_map should be land_map
e.g. ereefs:::map.df in R/sysdata.rda should be ereefs:::map_df

When users extract data, the units should be included for clarity, perhaps as attributes of the returned data frames. This would not be difficult, but might break backward compatibility.

Just to maintain good practice.
Repeated files and unwanted clutter to be added to a locally-ignored ignore folder.

I think it would be best to explicitly list the external packages and their functions via the importFrom tag from roxygen rather than internal ::.
It makes the dependencies more easily tracked.

Follow example from
https://github.com/ropensci/dataaimsr

Would this be a viable thing? Allow map_ereefs to take multiple time stamps as an argument and an additional argument to whether it should generate static maps, return data, or produce a gif.

Most functions start by setting up a bunch of variables, including start_date, start_day, start_month, start_year, end_date, end_day, end_month, end_year, input_file, input_stem, latitude, longitude, and sometimes mths, years, var_list, ereefs_origin. We should put all these variables into a list and then call a function to set them up.

See for example https://github.com/open-AIMS/bayesnec/blob/master/R/bayesnec-package.R

The official CRAN version of the ncdf4 package sometimes supports OPeNDaP-served netcdf4 files under Windows and sometimes doesn't. When it doesn't, the work-around is to install mdsumner/ncdf4 fork from github.

We could require this version, but might need to check it periodically. In the longer term, we might want to move to tidync instead.

Currently, the function generates a series of static image files and the user needs to use another program or function to stitch these together. Personally, I use imagemagick. This is now very easy to do within R using gifski https://github.com/r-rust/gifski

For some functions such as map_ereefs the map gets plotted twice, unless the user sets suppress_print, e.g.

This only happens when you do, e.g.

map_ereefs(target_date = c(2019, 2, 28))

instead of e.g.

p <- map_ereefs(target_date = c(2019, 2, 28))

The current solution to this is

map_ereefs(target_date = c(2019, 2, 28), suppress_print = TRUE)

But that goes against ggplots default behaviour to not print when assigned to an object

We need to add a series of tests to make sure intended functionality is correct, as well as anticipation of corner cases. Maybe a meeting could help discuss these through.

Would it be possible to also work with AIMS eReefs data service?
This is probably a high priority function as this might affect a lot the inner working of multiple functions.
If so we could create a function for this, or expand capacity of all functions.

This issue is not to be closed by any commit msg per se, but rather manually after we've conducted a manual review of the style throughout the functions. See:
http://adv-r.had.co.nz/Style.html

Some functions did not have descriptions to all arguments, and some repeated arguments did not have the same description across functions. These need to be checked and standardised.

The three examples in the get_ereefs_ts() function are throwing seperate errors:

get_ereefs_ts('Chl_a_sum', location_latlon=data.frame(latitide=c(-23.39189,-18), longitude=c(150.88852, 147.5)), layer='surface', start_date=c(2010,12,31),end_date=c(2011,1,5), input_file=2)

> Error in if (location_latlon[[1]] == "mmp") { : 
>   the condition has length > 1

get_ereefs_ts(var_names=c('Tricho_N', 'DIP', 'TP'), location_latlon=data.frame(latitide=c(-23.39189,-18), longitude=c(150.88852, 147.5)), layer='bottom', start_date="2012-07-03",end_date="2012-07-05", input_file='GBR4_BGC-v2.0 Chyb Dcrt')

> Error in if (start_date > end_date) { : 
>   missing value where TRUE/FALSE needed
> In addition: Warning messages:
> 1: In convert.dates(dates., format = format[[1]], origin. = origin.) :
>   days(s) out of range in positions 1 set to NA
> 2: In convert.dates(dates., format = format[[1]], origin. = origin.) :
>   days(s) out of range in positions 1 set to NA

get_ereefs_ts(var_names=c('ZooL_N', 'ZooS_N'), location_latlon=data.frame(latitide=c(-23.39189,-18), longitude=c(150.88852, 147.5)), layer=45, start_date=c(2010,12,31),end_date=c(2011,1,5), input_file="http://dapds00.nci.org.au/thredds/dodsC/fx3/gbr4_bgc_GBR4_H2p0_B2p0_Cpre_Dcrt/gbr4_bgc_simple_2016-06.nc")

> Error in if (location_latlon[[1]] == "mmp") { : 
>   the condition has length > 1

The third example still gives an error with the location_latlon when correcting latitide with latitude - changing location_latlon[[1]] to location_latlon[[1]][1] on line 19 is a temp fix to get around the data.frame issue:

 location_latlon=data.frame(latitide=c(-23.39189,-18), longitude=c(150.88852, 147.5))

 if (location_latlon[[1]][1] == "mmp") {
        location_latlon <- data.frame(latitude = mmp_sites$latitude, 
            longitude = mmp_sites$longitude)
        mmp <- TRUE
    }
    else {
        mmp <- FALSE
    }

> Error in if (location_latlon[[1]] == "mmp") { : 
>   the condition has length > 1

sessionInfo() below:

R version 4.2.1 (2022-06-23)
Platform: x86_64-apple-darwin17.0 (64-bit)
Running under: macOS Ventura 13.2.1

Matrix products: default
BLAS:   /Library/Frameworks/R.framework/Versions/4.2/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/4.2/Resources/lib/libRlapack.dylib

locale:
[1] en_AU.UTF-8/en_AU.UTF-8/en_AU.UTF-8/C/en_AU.UTF-8/en_AU.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] ereefs_3.2.4

loaded via a namespace (and not attached):
 [1] fansi_1.0.4      assertthat_0.2.1 utf8_1.2.3       dplyr_1.0.10     R6_2.5.1         DBI_1.1.3        lifecycle_1.0.3  magrittr_2.0.3   pillar_1.8.1     rlang_1.0.6      cli_3.6.0        vctrs_0.5.2     
[13] generics_0.1.3   glue_1.6.2       compiler_4.2.1   pkgconfig_2.0.3  tidyselect_1.2.0 tibble_3.1.8    

Currently the name map_ereefs_movie is misleading as it does not produce a movie file.
Suggestion would be to change it to map_ereefs_gif or something and generate a gif.

I think " might be best