Incomplete inner fields

[martclanor]

Hi,

Noticed a regression where inner fields (children of other fields) are sometimes excluded during the drilldown, except for the first child.

For example, when looking at the gwf-wel component, options block, afrcsv_filerecord field:

spec.components["gwf-wel"].blocks["options"].fields["afrcsv_filerecord"].fields.keys()
# Returns dict_keys(['auto_flow_reduce_csv'])
# Should return dict_keys(['auto_flow_reduce_csv', 'fileout', 'afrcsvfile'])

I checked the v2.0.0 snapshots, and this behavior seems to have been introduced in dev2.

Also, just to add more info, this doesn't seem to be the case for some other inner fields. For example:

spec.components["gwt-ssm"].blocks["sources"].fields["sources"].item.fields.keys()
# Correctly returns all children: dict_keys(['pname', 'srctype', 'auxname'])

[martclanor]

Quick update: FWIW, while tracing through the code, I noticed that the following line,

modflow-devtools/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py

Line 1308 in adf2f1a

rec_fields = _subfield_map()

is not being called reached and is probably related to the issue on this specific case. I'm still unsure how exactly to change things around here, but I thought I'd throw this in as it might offer a clue.

[aprovost-usgs]

@martclanor - Thanks for posting the issue and providing the update. The members of our team who can best evaluate this are currently out of the office. We'll follow up with a response as soon as we can.

[martclanor]

Thanks for letting me know, no rush at all!

[martclanor]

Found another relevant case in the utl-obs component where the binary field, among others, is missing. See autotest/dfns/__snapshots__/v2.0.0.dev3/utl-obs.yaml.

I believe this one is related to the fact that this line isn't wrapped with the try_parse_bool function:

modflow-devtools/modflow_devtools/dfns/migrate_to_v2_0_0_dev2.py

Line 1366 in adf2f1a

if field.get("in_record", False):

[wpbonelli]

Thanks for this @martclanor and sorry for the delay! You're right about the try_parse_bool issue.

Re: file records collapsing down to one field, that is intentional. The tentative new DFN schema has a first-class File type, mentioned in the spec here. The idea is a cleaner type system and a (mostly) format-agnostic spec. But I see there are a few issues:

1. The File fields lose the correct longname/description. The migration pulls from the leading tag field, not the path field.
2. The file record pattern is not universally TAG MODE PATH, there can be a trailing keyword flag and/or multiple leading tags, e.g.

• utl-obs.output is MODE PATH FLAG: no tag, trailing flag
• gwf-sfr.cross_sectionrecord is TAG TAG MODE PATH, two tag tokens for a single file path

I was hoping .render() would give you exactly what your plugin needs to show for hover text for each component, block, or field. Also useful for our docs. But I realize I didn't add a .render() method to the field types yet.

I'll work on all this and comment again here when it's in better shape. In general I'm curious whether you agree with the approach. Really value your feedback.

(Note the migration scripts are almost completely agent-generated, they can be thrown away once we're happy with the new spec/schema and switch MF6 over to it, so I've been playing pretty fast/loose with them. Sorry to make you wade through that.)

[wpbonelli]

Just a heads up. While working on render() I noticed your plugin only tags keyword fields, and ignores tagged for other fields. And it looks like it renders inline records as collapsed placeholders <record name> in all cases. So for instance GWF OC formatrecord hover looks like

[HEAD PRINT_FORMAT <formatrecord>]
<columns> <width> <digits> <format>

While in the MF6IO PDF it's fully rendered inline

HEAD PRINT_FORMAT COLUMNS <columns> WIDTH <width> DIGITS <digits> <format>

I think the collapsed placeholder makes sense for unions, since showing all the possible choices would be cluttered. The IO guide and the plugin are already consistent on this. E.g. GWF OC period block

[SAVE <rtype> <ocsetting>]
[PRINT <rtype> <ocsetting>]

But for records not part of a union, I'd say a full render makes sense?

[wpbonelli]

Re: file records collapsing down to one field, that is intentional. The tentative new DFN schema has a first-class File type, mentioned in the spec here. The idea is a cleaner type system and a (mostly) format-agnostic spec.

I've reconsidered. With #343, file records are now composites of zero or more Keyword fields plus a File field. I considered also making the mode/direction a Keyword too, but didn't. It's a bit tricky finding the right line to separate the abstract input model from MF6 input file format details, let me know if you have thoughts.

[wpbonelli]

Closing as the real omissions have been fixed by #343, #344, and #345, I think.. feel free to reopen if you find something else @martclanor, or separate issues to discuss any related design points.

[martclanor]

Sorry as well for the late reply! I will try this new implementation of .render() with fields.

Just to provide context to what I've been attempting to do: so far, I have only tried the new schema to recreate this file which is what the extension uses to look-up the field description for the mouse hover feature. The structure of the json file is basically: {field1: {block1: {description1: [component1, component2]}}. To recreate this, I needed to write some logic to get to the inner fields. This is an unpolished example of that.

The issues that I reported above are the first cases that I encountered based on the diff that I get on recreating the json. These are the ones I can't fix without upstream changes in modflow_devtools, e.g. the try_parse_bool wrap.I'm just thinking now, it would be nice to have such feature within modflow_devtools that enables iterating through all fields.

[martclanor]

I think the collapsed placeholder makes sense for unions, since showing all the possible choices would be cluttered. The IO guide and the plugin are already consistent on this. E.g. GWF OC period block

Yes, I agree with this. I was actually using the io guide as the target output especially for the block structure hover texts. I was doing this by manually comparing the hover texts with the block structures in the pdf. I realized soon that this is tough to get completely right and that certain blocks/fields are trickier than others. So when I got the hint that there is a planned rework on the underlying data structure, I held off on resolving the remaining inconsistencies in anticipation of handling it more systematically with modflow_devtools.

[wpbonelli]

Got it, so you have to build a reverse lookup table.. There is a get_fields() function but it only works on a component level, not by block.. I think a small modification to the schema, adding a Block.get_fields() method, would let you get rid of append and just have e.g.

for component in spec.components.values():
    for block in (component.blocks or {}).values():
        for name, field in block.get_fields(recurse=True).items(multi=True):
            if isinstance(field, (Record, Union, List)):
                continue
            hover_dict[name][block.name][field.description].add(component.name)