cvitolo / fuse Goto Github PK

This issue is part of this JOSS review

The README states install.packages(c("DBI", "assertthat", "magrittr", "tibble", "maptree", "tgp", "zoo", "qualV"))

But from DESCRIPTION the dependencies are
Imports: dplyr, zoo, tgp, stats
LinkingTo: BH, Rcpp
Suggests: testthat

Should the installation instructions be updated?

That said, I had no issue installing the package and am now running R CHECK 🎉

This issue is part of this JOSS review

This is again only a suggestion and quite similar to the suggestions made for the other datasets so I should maybe have grouped them, sorry.

• parameters could have a more specific name, e.g. fuse_parameters.

• the column names could be even more explicit (but I don't know about usual hydrology abbreviation so maybe I'm wrong), e.g. tishape could be topology_index_shape

Running a package check I get the following NOTE:

checking compiled code ... NOTE
File ‘fuse/libs/fuse.so’:
  Found ‘_ZSt4cout’, possibly from ‘std::cout’ (C++)
    Object: ‘fuse.o’
File ‘fuse/libs/fuse.so’:
  Found no calls to: ‘R_registerRoutines’, ‘R_useDynamicSymbols’

Compiled code should not call entry points which might terminate R nor
write to stdout/stderr instead of to the console, nor use Fortran I/O
nor system RNGs.
It is good practice to register native routines and to disable symbol
search.

Any idea how I could fix this?

I have added OSX to the list of operating systems to test on TRAVIS-CI, but it errors.

I do not have a mac, so any help to fix this is greatly appreciated!

This issue is part of this JOSS review

The title of the doc of fuserouting.ranges is "Function to define the parameter ranges for GAMMA routing module" but the function has no parameter, it only returns the default parameters so it's not for defining something.

• I'd propose to change the title to "default parameter ranges" or something like that.

• maybe the name of the function could even contain the word "default"?

This issue is part of this JOSS review

I'd recommend adding a NEWS file to the package (devtools::use_news_md() will create the template) for keeping track of changes introduced in the different version.

This issue is part of this JOSS review

Why not have the random seed as argument of fusesma.sim and fuserouting.sim and fuse (or of fuseonly since it calls both other functions), with a default value, to make results reproducible even if the user forgets about the seed?

It would be great to use BH package rather than having code in the src folder

This issue is part of this JOSS review

These are only suggestions and questions:

• I'd be in favour of naming modlist more explicitely, e.g. fuse_models, and maybe even without "list" in the name since it's a data.frame (I understand the reason for calling it list, but I guess that because list is an object class in R it might be confusing?).

• I do not understand the use of modliststring. When looking for its occurrences in the repo I could only find the documentation. If the only role of modliststring is to explain the codes, why are abbreviations used? Maybe it'd make sense to no longer have modliststring and instead have modlist and a data.frame per type of code with a column "code" and a column "meaning", or to have only modlist and its documentation. Or to have only modliststring and the documentation, i.e. use the abbreviations instead of the codes, and explain the abbrevations in the documentation?

• The names of the variables themselves could be a bit more explicit, e.g. esoil could be soil_evaporation. Unless those are all abbreviations commonly used in hydrology?

I have followed the instruction given in the README.md file. I have installed all the prerequisite packages also. But still i'm getting error at the end of installation.
The Error message is like this! I need some help regarding this issue. I'm using R version of 3.6.3

C:/RBuildTools/3.5/mingw_32/i686-w64-mingw32/include/c++/bits/random.h:2504:11: note: template class std::gamma_distribution
class gamma_distribution
^
fuse.cpp:1722:24: error: expected primary-expression before '>' token
gamma_distribution<> gd(shape);
^
fuse.cpp:1722:34: error: 'gd' was not declared in this scope
gamma_distribution<> gd(shape);
^
make: *** [C:/PROGRA~1/R/R-3.6.3/etc/i386/Makeconf:215: fuse.o] Error 1
ERROR: compilation failed for package 'fuse'

removing 'C:/Users/maniv/Documents/R/win-library/3.6/fuse'
Error: Failed to install 'fuse' from GitHub:
(converted from warning) installation of package ‘C:/Users/maniv/AppData/Local/Temp/Rtmp677Gkt/file26b027a519f1/fuse_3.0.0.tar.gz’ had non-zero exit status

This issue is part of this JOSS review

I read in the README (& paper) "Implementation of the framework for hydrological modelling FUSE described in Clark et al. (2008) and based on the Fortran code provided by M. Clark in 2011. "

I'm absolutely not a licensing expert, so I just wonder whether the original Fortran code was released (and if so under which license -- but I guess it wasn't released since you mention it was kindly provided?), whether the author agreed to its use for a R package with the license you've chosen? Or maybe it doesn't play a role since your code is not a binding but rather a translation (and even improvement?) of the original code? Had you considered adding him as a contributor of the package? But I see DESCRIPTION too mention the original paper so this issue is probably useless. :-)

This issue is part of this JOSS review

I wonder if the name of the function fuse is too confusing. Maybe you could rename it to something with "simulation" or "sim" in the name?

This issue is part of this JOSS review

This is only a suggestion: many exported functions of the package have a name starting with "fuse", why not all of them?

This issue is part of this JOSS review

Looking at codecov.io, it seems that the test coverage could be improved with a few more tests. :-)

This issue is part of this JOSS review

• Could you add a "similar packages" section to the README and vignette to mention the CRAN Task View environmetrics https://cran.r-project.org/web/views/Environmetrics.html and whether some packages do something similar or whether some packages have an output that could be used with fuse? This would help see how unique fuse is and whether there are alternative/complementary packages in R.

• When you submit the package to CRAN, I'd encourage you to contact the Task View maintainer to get fuse added to it but you might have already thought of this anyway.

This issue is part of this JOSS review

Because in that case you might want to mention that as example somewhere in the documentation?

This issue is part of this JOSS review

The most important remark of this issue: the doc states DATA is a data.frame but it's actually a zoo object, could you update the doc or the object itself?

These are details and only suggestions:

• The name "DATA" is not very specific, why not name the sample data fuse_hydrological_data or something like that? If it's a time series, maybe even fuse_hydrological_timeseries?

• The names of the columns/variables could be replaced by the full name of the variables, e.g. "precipitation" instead of P.

This issue is part of this JOSS review

A cache issue?

This issue is part of this JOSS review

The doc of fuserouting.sim should mention the class of the arguments, e.g. whether deltim is a numeric.

This issue is part of this JOSS review

I actually get an error now when running the tests,

1. Failure: Ensemble run (@test-fuse.R#54) -------------------------------------
all(discharges == y) not equal to TRUE.
1 element mismatch