Update(ModelTime): refactor ModelTime and add new features

[jlarsen-usgs]

• refactor call signature to accept perlen, nstp, and tsmult as parameters

• added methods:

  • set_start_datetime(): allows user to set/reset the start_datetime
  • set_time_units(): allows user to set/reset the time_units
  • timeunits_from_user_input(): uses fuzzy string logic to parse user supplied time units
  • datetime_from_user_input(): converts many types of datetime representation to a standard datetime object
  • get_totim(): allows user to get the totim value at the end of a stress period or stress period/time step combination
  • get_datetime(): allows user to get the datetime value at the end of a stress period or stress period/time step combination
  • intersect(): method that allows for time intersection. Method accepts datetime representations or totim and returns either the zero based stress period or the stress period and time step combination

• added testing for ModelTime

• updated FloPy model classes for ModelTime changes

[wpbonelli]

will this close #1915?

[jlarsen-usgs]

@wpbonelli

This will close #1915. The TemporalReference class had very few uses and it's functionality has been 1) replaced and 2) improved on in this PR.

[langevin-usgs]

Hey Josh, I had a bunch of little comments, many just about terminology. Just thought I'd bring them up for discussion before they get burned in. I think what you've done here will be widely used.

One thing to consider: we have adaptive time stepping now, which doesn't fit the mold of nstp/tsmult. Not sure how we should handle it, but I think we should leave space for it somehow. Unfortunately, we don't know the time stepping information until after the simulation runs, but we could have ModelTime load the information. We can modify ATS to write time step information, which might be helpful.

Is there a way to get a list of datetimes that correspond to the end of each time step? Seems like this would be great for making plots?

[langevin-usgs]

Should we consider dateutil.parser.parse here instead of rolling our own? If so, we could include dateutil as an optional dependency and benefit from any improvements made there. If so, should we use similar terminology and call this method parse instead?

[jlarsen-usgs]

I think we should consider it, but dateutil would need to be a core dependency because ModelTime is constructed much like the Grid classes on modflow model objects.

[jlarsen-usgs]

renamed this to parse_datetime

[langevin-usgs]

Suggested change

	def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgrive=False):
	def intersect(self, datetime_obj=None, totim=None, kper_kstp=False, forgive=False):

Note that forgive is misspelled in a few places.

Also, I wonder if intersect is the best name for this? Would this be better explained with find?

[jlarsen-usgs]

updated the forgive flag.

We should think about the naming here, because I think that this is going to be one of the more used methods of the class. I'm not sure that find is quite right for the method name

[langevin-usgs]

We have a method here called get_totim which means something different from the totim method. I think people might find this confusing. We should probably try and settle on a terminology, such as start/stop or begin/end. We might also need to think about a way to distinguish between datetime and elapsed time.

[jlarsen-usgs]

How about get_model_time or get_end_time?

[aleaf]

how about get_kstp_kper_totim(), since I think that's what it's doing? Also I think it's important to note that simulation time might be different from elapsed time, for example if the simulation starts with a steady-state stress period of some length.

[wpbonelli]

I know period data will be computed dynamically from perlen/nstp/tsmult now, but maybe still convenient to pass in a dataframe/recarray/list of tuples.. would it be hard to keep support for that?

[jlarsen-usgs]

It's possible to keep support for that, but if we added that too it would create multiple ways to instantiate the class. I personally think we should have a single standard way (either period_data or by each variable that would be in a period_data array). But I'm open to having multiple ways, it just complicates the code

[wpbonelli]

I'd personally vote to keep the initializer parameter a table, for backwards compatibility and because it is consistent with what we do for packages. We could have a from_arrays(perlen, nstp, tsmult) class method?

In future we could maybe also consider e.g. a from_times(times) class method doing what imod-python does

[aleaf]

consider refactoring test cases like this (here and in the functions below) to use @pytest.mark.parametrize. For example, this test could be refactored to:

@pytest.mark.parametrize('unit_name,user_inputs', (
    ("years", ["years", "YeaR", "yaEr", "ayer", "y", "yr", 5]),
    ("days", ["days", "Day", "dyAs", "dysa", "d", 4]),
    ("hours", ["hours", "Hour", "huors", "h", "hrs", 3]),
    ("minutes", ["minutes", "MinUte", "minte", "m", "min", 2]),
    ("seconds", ["seconds", "Second", "sedcon", "s", "sec", 1]),
    ("unknown", ["unkonwn", "undefined", "u", 0])
    ))  
def test_timeunits_userinput_parsing(unit_name,user_inputs):
    for user_input in user_inputs:
        mt_unit = ModelTime.timeunits_from_user_input(user_input)
        assert mt_unit == unit_name

This carries some advantages, in that each case is treated as a separate test (meaning all of the cases will be run unless "fail fast" is set), and the tests can be debugged individually in VS Code.

[aleaf]

for simplicity and readability, this could be shortened to simply

assert ModelTime.parse_datetime(dt_rep) == datetime.datetime(2024, 11, 12)

same for other checks with AssertionErrors

[aleaf]

I know intersect is used elsewhere, but consider renaming this method to say what it does, for example get_kstpkper() (if I'm understanding this correctly)