-
Notifications
You must be signed in to change notification settings - Fork 322
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
feat: add optional custom print callable #2121
feat: add optional custom print callable #2121
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2121 +/- ##
=======================================
Coverage 73.1% 73.1%
=======================================
Files 259 259
Lines 59526 59529 +3
=======================================
+ Hits 43531 43534 +3
Misses 15995 15995
|
Looks like the check "FloPy documentation / Prepare and test notebooks" timed out on Linux and MacOS. On Windows it takes 23m 22s but on Linux and MacOS it takes 6h 0m 16s. Both show the error message |
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.
Thank you @pya, I like this feature. The CI issue is resolved but maybe best to get some more buy-in before merging. @langevin-usgs @jdhughes @jlarsen-usgs @spaulins-usgs
flopy/mbase.py
Outdated
Optional callbale for printing. It will replace the builtin | ||
print function. This is useful for shorter prints or integration | ||
into other systems such as GUIs. | ||
default is None, i.e. use the builtion print |
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.
Just some typos
- callbale -> callable
- builtion -> builtin
Maybe also "shorter prints" -> "shorter output"?
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.
@wpbonelli Thanks for finding my typos. I guess, I should use a spell checker in my editor. ;)
I am not sure about the wording. "shorter output" seems a bit too generic. I would also associate something like file output with it. How about "shorter print output"?
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 "shorter print output" makes sense, or it could be merged as is if you prefer the first wording. I am happy to fix typos and merge, unless you are on it already?
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 fixed the typos and pushed my changes. Now ruff
died on the test file, which did not touch.
|
||
assert success | ||
assert any(buff) | ||
assert any(ws.glob("*.lst")) |
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.
Should we check buff
is the same when custom_print=print
and when not explicitly provided? Maybe also good to check that buff
changes with a custom callable.
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.
Thanks @pya. Seems like a good addition to me.
@pya rebasing on After some more thought, I wonder about the consequences of reassigning Apology for the delay, I thought I commented to the same effect last week, either I never clicked the button or it disappeared in the ether |
@wpbonelli Switching to ruff is a good thing. I just included a chapter about ruff in my teaching portfolio. I taught a short session about it last December. As for reassigning def run_model(
...
custom_print=None,
) -> Tuple[bool, List[str]]:
if custom_print is not None:
print = custom_print
else:
print = __builtins__["print"] I create a new local variable named If you don't like the name def run_model(
...
custom_print=None,
) -> Tuple[bool, List[str]]:
if custom_print is not None:
run_print = custom_print
else:
run_print = __builtins__["print"] Now, use |
@wpbonelli I rebased on develop with |
Very nice, my mistake- I misunderstood what was happening. Thank you for educating me :)
It sounds like your I tried locally and after rebasing, |
Since the `print` function is used, not changes in the output are expected.`
e0d7e29
to
e319098
Compare
Thanks for the hint. I updated my branch |
Thanks again @pya for this contribution |
Thanks @wpbonelli for walking me through the process. |
Objective
The printout of a MF6 run can be pretty long, especially if there are many time steps. While it is possible turn off printing altogether, it can be useful to allow the user to customize the printing. This PR suggest to add a callable the will be use during the model instead of the builtin
print
function.Example: Print time steps on top of each other
Setting up a model:
Defining a custom print callable:
Running the model with the custom printing:
Yields this output:
Over time the output develops like this:
A little bit later:
Of course other print variations are possible.
Impact
The impact on the code is minimal. Essentially handing through a custom callable and using it instead of
print
will do. No changes in behavior will occur if no callable is supplied.Testing
Running
pytest
yields the same result as without the changes.