Skip to content
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

refactor(codegen/docs): add type hints #2317

Draft
wants to merge 46 commits into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Sep 25, 2024

Sketching #2298, and possibly moving the templating in flopy/mf6/utils/createpackages.py to Jinja

Aiming to minimally/faithfully reproduce component source files, adding type hints in __init__ and the docstrings.

Type mapping

Not sure the mapping is right, but so far

list -> Iterable
keystring -> Union 
record -> Tuple (possibly variadic)
file record -> Tuple[Union[str, PathLike]]
array -> Iterable if of str, otherwise NDArray (ArrayLike in fn signatures)
double precision -> float
integer -> int
keyword -> bool (unless in a record, in which case Literal["..."])
string -> str (unless valid options specified, in which case Literal of options)

Array variables

I think while variables can be specified as NDArray with a type parameter, method/function implementations are lenient and NDArray can be switched to ArrayLike for type hinting. Conversely we might be able to use ndarray[Shape, T] where Shape = Tuple[str, ...] in a specification to defer shape checking until dimensions can be looked up from other parameters — this possible because numpy leaves the shape parameter wide open (it need not be a tuple of int)

Tagged records in keystrings

For records whose last item repeats, I considered doing:

  • Tuple[Any, ...]
  • Tuple[Union[Literal, T], ...]
  • Tuple[Literal, Tuple[T, ...]]

Ended up going with the latter.

Checking correctness

The correctness of this could be checked with tests or maybe mypy? Not sure about checking completeness — i.e. do the type hints truly cover all accepted types. For instance I think single-value records can be provided directly, without a wrapping tuple.

Todo

  • simulation
  • models
  • exchanges
  • subpackages
  • named tuples
    • I could see a case for or against these, as they make the type hint less explicit, but more concise
  • allow unwrapped 1-item tuples
  • accept NDArray as ArrayLike
  • description rendering
    • common variable substitutions
    • math symbols
    • citations
  • structural parity (AST checking)
  • functional parity, passing test suite
  • check correctness of type hints — mypy?

Motivation

This is a first step toward refactoring mfstructure.py and determining what is really necessary to represent an input data model. In the meantime this grows the codebase instead of shrinking it, but I am thinking of it a bit like scaffolding before renovation. Some complexity in the implementation is ascribable to the current framework and could simplify eventually.

wpbonelli added a commit to MODFLOW-USGS/modflow6 that referenced this pull request Oct 8, 2024
Add 'in_record true' to several record fields in utl-ts.dfn and utl-tas.dfn. Motivated by modflowpy/flopy#2317. FloPy can infer this, evidently, but a less clever (and ideally much simpler) implementation may require them explicitly. IMO it makes sense to do this anyway for consistency's sake, at least while the DFN specification is flat and record membership cannot be inferred directly from the spec's structure.

This is not necessarily comprehensive, further work may reveal the same (or similar) updates are needed elsewhere.
@wpbonelli wpbonelli changed the title refactor(codegen/docs): jinja templates, type hints refactor(codegen/docs): add type hints Nov 5, 2024
@wpbonelli wpbonelli added this to the 3.10 milestone Nov 5, 2024
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