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

Feature/install execute #12

Conversation

cneyens
Copy link
Collaborator

@cneyens cneyens commented Jan 30, 2020

This PR addresses #7 and in part #6. Since you're also working on this, I thought it better to PR into this branch instead of develop.

Changes:

  • rmf_install is updated. Installing examples is not (yet) supported. As mentioned in RMODFLOW extension packages #10, this might be better suited for a separate package.
    rmf_install will install the latest version of a MODFLOW code (either 2005, NWT, OWHM, LGR, CFP or all) for your OS or error out if not available. By default it will install in the "exe" folder of the RMODFLOW package library (created silently if it doesn't exist). Users can specify the dir argument if they want to install somewhere else. If a copy is already present in the directory, the console prompt will ask to overwrite (unless prompt = FALSE which was added if you want to install programatically without possible halting by a prompt).
    It differs from your branch in that it doesn't keep track of different versions in the same directory. If users want to have multiple versions of a MODFLOW code, they should change the dir argument. It also doesn't need the xml2 and rvest dependencies since the URL's are hardcoded.
  • As per Execute, optimize, analyze #7
    • rmf_run_modflow is changed to rmf_execute. It is also no longer an S3 method: it can only be run through by specifying the path to the NAME file. So you should first write the model, then run it. For the executable, by default it will look in the "exe" folder in the RMODFLOW package directory for the code specified by version. If nothing is found, an error is thrown asking to either specify executable or try calling rmf_install
      By default, a logical is returned specifying whether the model converged. This can be altered by setting convergence = FALSE. This was added to ensure rmf_optimize doesn't error out when the model does not converge during optimization.
      processx::run is used for calling the terminal command
    • rmf_run_opt is now rmf_optimize. It only uses the 'L-BFGS-B' method from the optim package. It can only be run on models having a PVL file and a HPR output file (see also Consistency with ModelMuse #11). The trans argument was changed to logtrans since log transformation was the only option available. Might need to be changed back.
    • rmf_run_sen is now rmf_analyze
    • rmf_run_rsm is removed
  • rmf_find is removed. Since users should either specify the executables directly or RMODFLOW will look by default in the "exe" folder in the package directory, this function seemed redundant. Please correct me if I'm wrong.

Documentation is not build yet. Will do after the merge into develop together with all the functions there.

Dependencies changed:

  • Removed hydroPSO, DEoptim, xml2 & rvest
  • Added processx

@rogiersbart
Copy link
Owner

rogiersbart commented Jan 30, 2020

Hi Cas,

thanks for doing this now! I wasn't fully aware apparently you had something coming up as well. Will have a look and try to merge your work with some things I had in mind. Some reactions on what I read above:

  • Great you removed xml2 and rvest as well!
  • Not sure about the dir argument for installing. Will have to see how you handle then keeping track of that. My idea was to have an option with the path that should be used, which, if not there, is set to the default package folder installation location. In this case, you avoid that people run code of others, and things get installed somewhere outside of the R package library (think I remember reading somewhere this is not very friendly behaviour). Doing it through the option requires every user to set it explicitly, if they want it to be in some other location. In this case rmf_find still makes sense I think, but it doesn't have to be exported. Can be an internal function.
  • only bfgs makes sense to me, if relevant, other optim options could be part of eg RMODFLOW.invert as well
  • requirement of an hpr file (this is again violating ModelMuse conventions by the way :) -> should be hob_out I think) is not sufficient I think. Will probably look into that soon. Should be possible to select any of the observation process packages.
  • the rmf_execute S3 method still seems useful for me, in case you have a full model in memory, through the top-level functionality you implemented (haven't looked at that in detail yet), and want to run things, and get all the output back in memory. Not sure if this will be so useful in practice, but definitely for small test models, if we would go to testing, it would be very interesting if this was still possible.

rogiersbart added a commit that referenced this pull request Jan 30, 2020
@rogiersbart
Copy link
Owner

Ah, and to avoid double work in future, maybe try to assign yourself to the corresponding issue! ;) I think I will try to have you review my feature branches from now on through pull request to develop as well, so then you'll always be aware of changes, and two minds reflecting on potential use cases etc. is very useful I think.

@cneyens
Copy link
Collaborator Author

cneyens commented Jan 30, 2020

I indeed did not assign myself to #7 or #6; my bad. I'll have some more time over the weekend to go through your comments. I'd like to get to v0.5.0 asap since it seems that I've infected some of the people around me to start using RMODFLOW as well ;) (researchers + students)
Please also note that I'm working on some small bug fixes/updates as an continuation of #8 in a separate branch

@rogiersbart rogiersbart merged commit d90230d into rogiersbart:feature/install-and-execute Jan 30, 2020
@rogiersbart
Copy link
Owner

rogiersbart commented Jan 30, 2020

Ok, great! And no problem for #8. Not immediately working on that, except maybe here and there making function names consistent with ModelMuse.

Just merged by the way. Not sure if I broke anything. Will try to finalize this asap, so we can merge in develop.

@cneyens
Copy link
Collaborator Author

cneyens commented Feb 2, 2020

Not sure about the dir argument for installing. Will have to see how you handle then keeping track of that. My idea was to have an option with the path that should be used, which, if not there, is set to the default package folder installation location. In this case, you avoid that people run code of others, and things get installed somewhere outside of the R package library (think I remember reading somewhere this is not very friendly behaviour). Doing it through the option requires every user to set it explicitly, if they want it to be in some other location. In this case rmf_find still makes sense I think, but it doesn't have to be exported. Can be an internal function.

My idea was that by default, rmf_install will install in the RMODFLOW package directory (and by default, rmf_execute etc would look there for the executable). If users don't want that, they can specify dir (and correspondingly executable for rmf_execute). It would then be their responsibility to make sure those paths are correct. But I see now that setting the dir argument might indeed give problems with reproducibility. Setting through an option might therefore indeed be a better idea.

only bfgs makes sense to me, if relevant, other optim options could be part of eg RMODFLOW.invert as well

So withhout lower/upper bounds as discussed in #7 ?

requirement of an hpr file (this is again violating ModelMuse conventions by the way :) -> should be hob_out I think) is not sufficient I think. Will probably look into that soon. Should be possible to select any of the observation process packages.

Correct but right now, RMODFLOW only supports HOB and its output...

the rmf_execute S3 method still seems useful for me, in case you have a full model in memory, through the top-level functionality you implemented (haven't looked at that in detail yet), and want to run things, and get all the output back in memory. Not sure if this will be so useful in practice, but definitely for small test models, if we would go to testing, it would be very interesting if this was still possible.

Ok. Docs should explain the different use-cases then. I think most users would simply run rmf_execute(nam_path)

@cneyens cneyens deleted the feature/install-execute branch September 12, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants