-
Notifications
You must be signed in to change notification settings - Fork 3
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
UZF, RCH, EVT, WEL and MAW improvement #263
Conversation
Distribute k based on kh-value in wel_from_df Add multiplier to wel_from_df, so q can be a timeseries Add aux to maw_from_df Improve speed by using gdf_to_grid instead of slow gwf.modelgrid.intersect becuase it is fast, remove tqdm maw-cells can be added to top layer as well (was not possible before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Just one suggestion and an unfinished comment.
@@ -840,6 +836,31 @@ def lcid_to_reclist( | |||
return reclist | |||
|
|||
|
|||
def cols_to_reclist(ds, cellids, *args, cellid_column=0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at some point this function should replace the lrc_to_reclist
and the lcid_to_reclist
functions because they something similar. I would not do it now but I can make an issue of this if you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree!
nlmod/gwf/wells.py
Outdated
|
||
|
||
def _get_layer_multiplier_for_well(cid, well_top, well_bot, ml_top, ml_bot, ml_kh): | ||
"""Get a factor for each layer that a well""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sentence not finished
nlmod/gwf/gwf.py
Outdated
logger.info("creating mf6 UZF") | ||
|
||
# create uzf package | ||
evt = recharge.ds_to_uzf(gwf, ds, pname=pname, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call it uzf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Lots of improvements, I left some comments to consider, but nothing major.
nlmod/gwf/recharge.py
Outdated
A DataArray with integer values, set to one for land surface cells indicating | ||
that boundary conditions can be applied and data can be specified in the PERIOD | ||
block. A value of 0 specifies a non-land surface cell. Landflag is determined | ||
from ds when it is None. The default is None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this determined?
nlmod/gwf/recharge.py
Outdated
The potential evapotranspiration rate of the UZF cell and specified GWF cell. | ||
Evapotranspiration is first removed from the unsaturated zone and any remaining | ||
potential evapotranspiration is applied to the saturated zone. only | ||
used if SIMULATE_ET. When passed as string, the array is obtained from ds. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simulate_et=True
nlmod/gwf/recharge.py
Outdated
None. | ||
extwc : float, optional | ||
The evapotranspiration extinction water content of the UZF cell. Only used if | ||
SIMULATE_ET and UNSAT_ETWC. Set to thtr when None. The default is None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention these should be set to True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe nitpicking, but I think this makes it more clear these values should be set in in this function?
nlmod/gwf/recharge.py
Outdated
The depths at which observations of the water depth in each cell are added. The | ||
user-specified depth must be greater than or equal to zero and less than the | ||
thickness of GWF cellid (TOP - BOT). | ||
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the intention, but we should explain that this creates a CSV output file with water contents for each UZF cell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obs_z
is missing from the docstring
user-specified depth must be greater than or equal to zero and less than the | ||
thickness of GWF cellid (TOP - BOT). | ||
The | ||
** kwargs : dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any kwargs left :P?
|
||
# perioddata : [iuzno, finf, pet, extdp, extwc, ha, hroot, rootact, aux] | ||
finf_name_arr, uzf_unique_dic = _get_unique_series(ds, finf, "finf") | ||
finf = "rch_name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an internal data variable? One that is set by nlmod functions? Maybe we should consider some convention for naming these internal data variables, to make it clear this is something nlmod has created and needs for building models?
Are there any other similar data vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the approach from RCH and EVT. However, I think we should not add these variables to ds. I will make an issue about this.
nlmod/gwf/recharge.py
Outdated
raise (ValueError("There cannot be nan's in the DataFrame")) | ||
# set the first value for the start-time as well | ||
df.loc[pd.to_datetime(ds.time.start)] = df.iloc[0] | ||
# combine the values with the start of each period, and ste the last value to 0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo ste = set
ml_bot = gwf.dis.botm.array | ||
kh = gwf.npf.k.array | ||
layer = range(gwf.dis.nlay.array) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe raise an Exception if both are None?
nlmod/gwf/wells.py
Outdated
# the well does not cross any of the layers. Just return an array of zeros. | ||
multiplier = kd | ||
else: | ||
# devide by the total kd to get a factor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo divide
wells.loc[1] = 200, 150, -20, -30, -300.0 | ||
|
||
sim, gwf = get_sim_and_gwf() | ||
nlmod.gwf.wells.wel_from_df(wells, gwf, auxmultname=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between the auxmultname=None and setting it to "multiplier"? In both cases a multiplier is applied, but either via AUX variables or just directly in the perioddata?
Maybe a docstring in wel_from_df() would help clear that up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I added a docstring.
This PR adds the UZF package with a notebook. Also it adds some improvements to RCH and EVT packages, like adding an aux-parameter for the concentration of recharge.
Another improvement is in the MAW and WEL packages. This should have been another PR, but it isn't. These improvements are: