-
Notifications
You must be signed in to change notification settings - Fork 6
fix(gwf): add field default for package singletons #245
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
Conversation
wpbonelli
left a comment
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, this goes part of the way to addressing Joeri and Marnix's point about it not being clear what is/isn't required when constructing (any subset of) a simulation.
flopy4/mf6/gwf/__init__.py
Outdated
| ic: Ic = field(block="packages") | ||
| oc: Oc = field(block="packages") | ||
| npf: Npf = field(block="packages") | ||
| ic: Ic = field(block="packages", default=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.
could also modify the type hints like Ic | None for each of these singles, to make clear the package isn't required in e.g. the gwf model initializer.
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.
do you mean like ic: Ic | None = field(block="packages", default=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.
Yeah exactly.
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.
When I try that there seems to be a xattree issue, here's the unaltered quickstart test result:
cls = <class 'flopy4.mf6.gwf.ic.Ic'>
def _find_field(cls: type) -> str:
matches = set()
for name, field in parent_spec.items():
if isinstance(field, Child) and isclass(field.type) and issubclass(cls, field.type):
matches.add(name)
match len(matches):
case 0:
> raise TypeError(
f"Class '{parent_cls.__name__}' has no fields of type {cls.__name__}"
)
E TypeError: Class 'Gwf' has no fields of type Ic
../.pixi/envs/dev/lib/python3.11/site-packages/xattree/__init__.py:734: TypeError
Maybe this is related to top down initialization- gwf is created without Ic and then passed in as the parent when Ic is created.
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.
Ah, shoot. I think it's probably failing to parse the child type correctly when it inspects the class to build the xatspec, then getting confused when it can't find any Ic children. The introspection pass probably doesn't "unwrap" optionals (i.e. union of something and None) correctly. I'll have a look later, unless you want to. It'll be in _get_xatspec somewhere but that is a prickly pile of code.
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.
Sorry I'm on mobile so might be wrong, but I think if right below this line, still in the optional condition https://github.com/wpbonelli/xattree/blob/develop/xattree%2F__init__.py#L628 if we do if has(type_) to see if the unwrapped type is another xattree cls, then set is_child = True and child_kind = "only"? That might do it?
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.
Ok it's fixed in xattree, if you update the pixi env it should be good to go.
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!
maybe this means npf and ic should not set the default? some packages are implicitly required by mf6 |
yeah, then those shouldn't have the optional type hint or the none default. do we know which packages are required? I think this came up previously and you said the IO guide doesn't explicitly state this. I don't see it either. It does say how many of certain packages are allowed. |
Yeah maybe we should leave everything optional (except dis?) for now and deal with that somehow in the spec when it becomes relevant to distinguish. |
37c550e to
0e96c82
Compare
fix #230