feat(solvers): support for multiple solver types (#1706) #1709
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1709 +/- ##
=========================================
- Coverage 71.9% 71.9% -0.1%
=========================================
Files 253 253
Lines 55890 55927 +37
=========================================
+ Hits 40205 40227 +22
- Misses 15685 15700 +15
|
There was a problem hiding this comment.
Thanks for this @spaulins-usgs. Couple of minor comments to think about.
flopy/mf6/modflow/mfsimulation.py
Outdated
| def register_ims_package(self, solution_file, model_list): | ||
| warnings.warn( | ||
| "register_ims_package() has been deprecated and will be " | ||
| "removed in version 3.3.7. Use " | ||
| "register_solution_package() instead.", | ||
| DeprecationWarning, | ||
| ) | ||
| self.register_solution_package(solution_file, model_list) | ||
|
|
There was a problem hiding this comment.
I don't think we want to deprecate this yet. Can we keep it for now, and reconsider when we officially release another solver type?
There was a problem hiding this comment.
@langevin-usgs, why not give the user as much warning as possible when a method will be deprecated? Are you thinking that you might come up with a better name than "register_solution_package" and don't want to commit to that name yet?
There was a problem hiding this comment.
EMS is experimental now and while we're pretty sure it will come out at some point, it's premature to talk about deprecating existing options in order to support it. For the medium term, I don't see us having more than two types of solutions, so continuing to support register_ims_package() seems fine for now.
There was a problem hiding this comment.
@langevin-usgs, deprecation warning removed
flopy/mbase.py
Outdated
| exe = which(exe_name) | ||
| if exe is None: | ||
| if exe_name.lower().endswith(".exe"): | ||
| # try removing .exe suffix | ||
| exe = which(exe_name[:-4]) | ||
| if exe is None: | ||
| # try abspath | ||
| exe = which(os.path.abspath(exe_name)) | ||
| if exe is None: | ||
| raise Exception( | ||
| f"The program {exe_name} does not exist or is not executable." | ||
| ) | ||
| if os.path.dirname(exe_name) == "": | ||
| exe = which(exe_name) | ||
| if exe is None: | ||
| if exe_name.lower().endswith(".exe"): | ||
| # try removing .exe suffix | ||
| exe = which(exe_name[:-4]) | ||
| if exe is not None: | ||
| exe_name = exe_name[:-4] | ||
| if exe is None: | ||
| raise Exception( | ||
| f"The program {exe_name} does not exist or is not executable." | ||
| ) | ||
| else: | ||
| if not silent: | ||
| print( | ||
| f"FloPy is using the following executable to run the " | ||
| f"model: {exe}" | ||
| ) | ||
| else: | ||
| if not silent: | ||
| print( | ||
| f"FloPy is using the following executable to run the model: {exe}" | ||
| exe_name = os.path.abspath(exe_name) | ||
| if not os.path.exists(exe_name) and not os.path.exists( | ||
| f"{exe_name}.exe" | ||
| ): | ||
| raise Exception( | ||
| f"The program {exe_name} does not exist or is not executable." | ||
| ) | ||
| else: | ||
| if not silent: | ||
| print( | ||
| f"FloPy is using the following executable to run the " | ||
| f"model: {exe_name}" | ||
| ) |
There was a problem hiding this comment.
It's great to have this change, but I think we want this one as a separate PR. I understand that that it's easy to sneak it in here, but I think a separate PR is warranted in this case.
There was a problem hiding this comment.
@langevin-usgs, rolled back exe path feature for another PR
feat(solver support): rolling back ims deprecation warning
Also included: fix(exe path): FloPy now correctly resolves relative paths to mf6 executable (#1633)