Skip to content

refactor(modeltime): use dataclass, add type hints #2528

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Jun 11, 2025

This allows removing some boilerplate. Also:

  • Add type hints
  • Flesh out some docstrings
  • Slightly more robust error handling in the initializer
  • Coerce input arrays to the expected dtype with astype
  • Fix an issue where the default tsmult value was int, not float
  • Support passing scalars to the initializer for a single stress period
  • Implement __eq__. Unfortunately we can't use the one from dataclass, lists play nice with == but not numpy arrays, and dataclasses.field doesn't let you override the comparator. However, attrs does and we could make ModelTime an attrs class when we bring that dependency in, as planned for 4.x.

A caveat. This sacrifices the ability to parse from string when setting time_units and start_datetime after initialization. dataclasses.field doesn't support converters as attrs.field does. Do we expect people to mutate ModelTime? It's easy enough to create a new one — it could even be a frozen dataclass. If we want it to stay mutable, I will just close this for now and reconsider when attrs is available. Or we could just bring in attrs now. It is dependency free.

Copy link

codecov bot commented Jun 11, 2025

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.

Project coverage is 55.5%. Comparing base (556c088) to head (c5ca5dc).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
flopy/discretization/modeltime.py 87.3% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2528   +/-   ##
========================================
  Coverage     55.5%    55.5%           
========================================
  Files          644      642    -2     
  Lines       124135   124207   +72     
========================================
+ Hits         68947    69016   +69     
- Misses       55188    55191    +3     
Files with missing lines Coverage Δ
flopy/discretization/modeltime.py 75.7% <87.3%> (-1.3%) ⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wpbonelli wpbonelli marked this pull request as ready for review June 11, 2025 19:34
@wpbonelli wpbonelli requested a review from jlarsen-usgs June 11, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant