Methods and tests for setting stream bed elevations #27
Conversation
Minor updates to tests after refactoring Mods and bugfixes also after refactoring Quick improvements to plotting feedback for stream elev and model top stuff
TODO: * Consider makeing more use of segment division to allow info at segment level to propagate better, this is closer to the prvious approach.. * Add support for user explicitly setting desired elevations of reaches/segments, e.g. max elevations etc. (And propagate through segment elevation changes to reach definitions.) * Consider using method the moves upstream (from outflow) -- may resolve tendancy for auto incision to increase downstream (more physical for incision to decrease downstream (generally?)) * Obvs DOCS! (notebook anyone?)
+ quick docstring
added option for setting rtp
- Use new functions in tests rather than deprecated ones - Show DeprecationWarning messages too (perhaps a bit much?)
* proper setting of updated botm using mf6.data.set_data() * remove comment * little bug fix in botm adjustment
- Fix docstring for plot_profile - Add draw_lines parameter for plot_reaches_vs_model
| """ | ||
| if segbyseg: | ||
| raise NotImplementedError | ||
| self._segbyseg_elevs(minslope, fix_dis, minthick) |
There was a problem hiding this comment.
Should SwnMf6._segbyseg_elevs (currently at L628) be included? It's not used/tested, and will obviously not work, as this structure does not have segment_data (which is exclusive to SwnModflow). We can remove it for the final merge, but keep the code in this PR.
There was a problem hiding this comment.
I think conceptually it could still be an option. We still have segments. The reach by reach elevation corrections are necessarily (loosely) "pinned". The idea was that if line segment tops and bottoms were available we would try to use these to constrain the reach elevations. Without this we only use the upstream and downstream elevations off each line path. I'm happy for it to stay in as it might be something that we want to implement and the bones of the code that is there could hep with that.
There was a problem hiding this comment.
Great, we'll merge it to main, but raise NotImplementedError
swn/modflow/_base.py
Outdated
| @@ -1182,3 +1193,155 @@ def firstpt(g): | |||
| ax=ax, label="diversion", marker="D", color="red") | |||
|
|
|||
| return ax | |||
|
|
|||
| # __________________ SOME ELEVATION METHODS_________________________ | |||
| def add_model_topbot_to_reaches(self, m=None): | |||
There was a problem hiding this comment.
Is there any reason why m should be a variable here? Preferably there is only one model from the self.model property. I realise this is a renamed function where m was available, but could it be removed in this function?
There was a problem hiding this comment.
yep, something of a legacy. If self.model will always exist. Then can be dropped. Same probs goes for
and the call to this function in there...There was a problem hiding this comment.
Ok, I've removed the parameter, which will raise a TypeError if it's supplied.
There was a problem hiding this comment.
I probs just had it so that my own codes didn't break but shouldn't be too hard to update!
| layerbots[k + 1] = layerbots[k] - laythick | ||
| self.model.dis.botm.set_data(layerbots) | ||
|
|
||
| def _reachbyreach_elevs( |
There was a problem hiding this comment.
There is a considerable amount of code overlap between SwnMf6._reachbyreach_elevs and SwnModflow.fix_reach_elevs. Should some of this logic be moved somewhere to SwnModflowBase, where reaches is common? It might need to go into a private _-prefixed function, as the parameters are different.
There was a problem hiding this comment.
Yep, a lot of similarities. However, subtle but critical differences. This method in SwnMf6 is essentially a re-write of SwnModflow.fix_reach_elevs but changed to loop over line path rather than segments. There is overlap for sure but the actual functions differ, there is a danger that the method becomes very fragmented if we try and pull out common elements into an abstract base.
There was a problem hiding this comment.
I'll postpone this task for later.
| layerbots[k + 1] = layerbots[k] - laythick | ||
| self.model.dis.botm.set_data(layerbots) | ||
|
|
||
| def _to_rno_elevs( |
There was a problem hiding this comment.
I don't think there is any testing coverage here. Is it possible to add/modify a test to get this method tested?
There was a problem hiding this comment.
Postpone for now, but I'd be nice to have coverage at some point.
There was a problem hiding this comment.
@wkitlasten may be able to put a test together for this... maybe a renaming would help clarify the method too?
|
@mwtoews I wonder if we could get the logger messages to print to a designated log file? They can be useful to check that thing aren't going silly. |
* Remove "m" (model) parameter from: - SwnModflowBase.add_model_topbot_to_reaches - SwmModflow.get_top_elevs_at_segs - SwmModflow.set_topbot_elevs_at_reaches * Raise NotImplementedError for: - SwnMf6.fix_reach_elevs(segbyseg=True) - SwnMf6._segbyseg_elevs Also, improve a few more docstrings
|
According to the logger module docs for Logging to a file, it should be a matter of: this seems to work? There probably is a better way, but the logger module is dense to look at. Otherwise, this can be toggled: (these only take effect for new objects) |
For sane stream reaches set downstream and within model top layer