PecanProject / pecan

The Predictive Ecosystem Analyzer (PEcAn) is an integrated ecological bioinformatics toolbox.
www.pecanproject.org
Other
203 stars 235 forks source link

`write.ensemble.configs` fails if `write.to.bety` is `FALSE` #2244

Open ashiklom opened 5 years ago

ashiklom commented 5 years ago

At issue is this code block: https://github.com/PecanProject/pecan/blob/bb018557ccecfeed4f76d02a154ed9a147ed93a4/modules/uncertainty/R/ensemble.R#L211-L220

Even if we are not writing to BETY, we should still be able to read from it, but we are unable to do so because con is set to NULL. Really, the only circumstance where con should be set to NULL is if the entire database settings block is missing.

This leads to some insidious downstream bugs, as @serbinsh and I discovered. For instance, none of the input paths required for ED2 (e.g. css, pss, site, thsum) are set, because without a database connection, we are unable to determine what the required tags for a model are in this block: https://github.com/PecanProject/pecan/blob/develop/modules/uncertainty/R/ensemble.R#L247-L259

serbinsh commented 5 years ago

For some addition info see: Issu #2245

ashiklom commented 5 years ago

Also, minor note, but there are a bunch of style guide violations in this code that makes it hard to read. For future reference, please make sure you enable RStudio's built-in code diagnostics (specifically, the "Provide R style diagnostics) option and try to address most of the issues it highlights (you may also need to install the lintr package for this to work properly). (Or, if you use a different text editor, enable some kind of asynchronous code linter. Pretty much any editor worth its salt will have a nice package for this).

serbinsh commented 5 years ago

@para2x bump.....could you take a look at this and let us know if this is an easy change/fix?

para2x commented 5 years ago

@serbinsh The reason I haven't taken any actions is because I haven't run ED yet and it's hard for me to test and see if my solution would work (and spending time on learning on how to run ED is not what I want to spend time on at the moment). But I can have a fix PR and let you guys test it.

dlebauer commented 5 years ago

@para2x have you tried running the integration tests? They are intended to be able to be run without having to understand how to set up a specific model. For example, you should be able to run ED2 on the VM with the following code:

./web/workflow.R --settings tests/pecan64.ed.xml

In this case, you will need to set the

  <database>
    <bety>
       ...
       <write>TRUE</write>
KristinaRiemer commented 5 years ago

FYI, I just ran into the bug in #2245. Got over that hurdle by changing the config file according to @dlebauer suggestion, though I'm still working through the rest of the bugs.

github-actions[bot] commented 4 years ago

This issue is stale because it has been open 365 days with no activity.