Multiple improvements #289
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update modpath executable Add nlmod.plot.geotop_lithok_on_map() Add "concentration" to perioddata in nlmod.gwf.buy() Add optional "layer" argument to extrapolate_ds, so you can use method for geotop-data with layer="z" as well
See comment from Onno in issue #275
…in top of regis and geotop And fix codacy issue
And fix some tests Fix extrapolate_ds for 2d-data-arrays
…metries anymore Change to HHNK
And improve test
Merged
dbrakenhoff
reviewed
Nov 17, 2023
There was a problem hiding this comment.
Looks good!
Two comments to discuss:
- one about the naming of the fill_top_and_bottom() method and what it does.
- can we think of names for the two layer model data models that we can reference by name, and explain somewhere in the documentation. e.g. "full-3D layer model" and "Modflow layer model". Maybe we can also use those names in our function names
convert_to_modflow_layermodelandconvert_to_3d_layermodel(). And in that explanation we should show how inactive zones can be derived from the thickness attribute. Come to think of it we should maybe add a method,get_active_domain(ds, layer=None).
Anyway, those were my thoughts.
| return qm | ||
|
|
||
|
|
||
| def _add_geotop_lithok_legend(lithok_props, ax, lithok=None, **kwargs): |
| return ax.legend(handles=handles, **kwargs) | ||
|
|
||
|
|
||
| def _get_geotop_cmap_and_norm(lithok, lithok_props): |
| ) | ||
|
|
||
| # make sure the total thickness of the two models is equal (does not assert to True yet) |
There was a problem hiding this comment.
No idea yet... It is a difference of 0.125 if I sum the total thickness. I have no idea where this comes from.
tests/test_009_layers.py
Outdated
| ds2["botm"].loc[new_layer].data[mask] == ds1["botm"].loc[layer].data[mask] | ||
| ).all() | ||
|
|
||
| # make sure the top of the new layer is euqal to the top of the original layer |
fix split_layer_ds and combine_layer_ds, and add tests small fixes to notebooks
…into multiple_improvements
dbrakenhoff
reviewed
Nov 20, 2023
| plot_test(regis, regis_split, colors=colors) | ||
|
|
||
|
|
||
| def test_add_layer_dim_to_top(): |
| @@ -450,13 +450,17 @@ def facet_plot( | |||
| if iper is None: | |||
| raise ValueError("Pass 'period' to select timestep to plot.") | |||
dbrakenhoff
approved these changes
Nov 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request does the following:
extrapolate_dsaccepts alayerargument, so you can use it for geotop data as wellextrapolate_dsfor 2d (non-layered) datanlmod.plot.geotop_lithok_on_map(tested in tests/test_002_regis_geotop.py)So the most important change is the first:
nlmod.read.get_regis(extent)will return a Dataset without a layer dimension in the variabletop(which can be overridden by settingdrop_layer_dim_from_topto False by the way). Does everybode agree?