Fix indexed array assignment + indexed derived types and a bit more

[ken-lauer]

Context

Closes #110
Closes #127
And maybe others?

f90nml was failing miserably for namelists from this charged particle physics simulation library Bmad, and this PR is the result of my poking around into trying to address that. (See all of the tao.init files in that repository, they are all Fortran namelists, many of which fail to parse with the current master)

So this ended up being a big set of changes and fixes. I started out with attempting this entirely manually, but admittedly did get Claude's help here. If that's against your contribution rules, I completely understand! Perhaps it at least can be a basis for a better fix.

If that's still OK, then I expect you might want me to try breaking up this PR into several. I can make an attempt, if there's a greater chance for these changes to get merged.

Changes / fixes

• Parent-indexed derived-type slice assignment: arr(1:N)%foo = v1, ..., vN now parses into arr.foo = [...] and round-trips back to the slice form.

• Open-ended parent-indexed bound:
  var(1:)%ele_name = ... (and var(:N)%...)
  gets the bound inferred from the value count instead of crashing/dropping data.

• Single-value open-ended slice:
  var(1:)%ele_name = 'x' correctly produces a one-element list

• Sliced field plus per-element field on the same array:

  var(1:6)%ele_name = ...
  var(1)%low_lim = 0.001
  var(3)%low_lim = 0.001
  

• Positional derived-type rows, which can have mixed types:

  datum(1) = 'a', '', 'M1', 4.5
  

• Positional then field on the same index - this is stored as _positional_row:

  design_lattice(1) = 'bmad.lat'
  design_lattice(1)%dynamic_aperture_calc = .true. now keeps both
  

• Unindexed positional then field:

  f = 'hello'
  f%x = 42
  

• Data getting scattered into different, previously-parsed derived type lists:
  For example, after var(1)%ele_name, var(2)%ele_name, var(3)%ele_name are set, a follow-up var(1:3)%attribute = 'k1', 'k2', 'k3' distributes values
  into the existing elements rather than overwriting them.

• Nested parent-indexed derived types, along with deeper nesting (hopefully):

  arr(1:2)%inner%foo = 1.0, 2.0
  

• Strided parent index:

  arr(1:5:2)%foo = 10.0, 30.0, 50.0
  

  Parses into [10, None, 30, None, 50] and round-trips back to the strided form with a bit of stride detection logic.

• Multi-value indexed assignment:

  var(N) = v1, v2, ..., vK
  

  This is treated as a positional row N instead of a flat one-dimensional vector starting at N.

• FIndex debug repr: added __str__ so I could see what was going on with it.

There's additional metadata for keeping track of what's positional, when slicing was used, when parent-based indexing was used. The "_positional_row" value is special in the value dictionary, as when you combine positional derived type setting and fieldname-based settings they can clash (... why do people do this and why does Fortran allow it? Ugh.).

The additional metadata helps with round-tripping back to namelist files especially.

I admittedly did not get to testing patching with these, but it's past 5PM on a Friday. So I think that's good enough for now.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.29%. Comparing base (34da4c8) to head (044ffec).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
+ Coverage   97.22%   97.29%   +0.06%     
==========================================
  Files           9        9              
  Lines        2166     2365     +199     
  Branches      361      416      +55     
==========================================
+ Hits         2106     2301     +195     
- Misses         35       39       +4     
  Partials       25       25              

Flag	Coverage Δ
python2.7	?
python3.5	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marshallward]

Thank you so much, this is an epic contribution. Derived type array assignment has been ignored for a very long time because the original code was never designed to handle it, and it happened to be rare enough that there wasn't much demand.

It will take me some time to process all of it, but overall it looks good to me.

As for acknowledging Claude, this advice seems like the best to me:

https://docs.kernel.org/process/coding-assistants.html

Something in the commit log like this

Assisted-by: Claude:claude-3-opus

should be more than enough. If you tell me the LLM that was used, I can squash the commits and append it to the end. (Or, if you wanted to rebase the commits with documented commit messages, that would also be great, but not necessary.)

[ken-lauer]

Thank you so much, this is an epic contribution. Derived type array assignment has been ignored for a very long time because the original code was never designed to handle it, and it happened to be rare enough that there wasn't much demand.

I'm glad you think so - and I hope this is a good basis for full support. It's pretty close, I think.

It will take me some time to process all of it, but overall it looks good to me.

I completely understand. I would typically complain if someone gave me such a large PR to review, and here I am proposing this as a first time contributor...

As for acknowledging Claude, this advice seems like the best to me:
[...]

I rebased and marked the commits that were largely LLM-assisted as you requested. If this ends up getting squashed into one commit with that attribution, that's OK with me - it's your call as to if/when/how to take this in.

[ken-lauer]

A polite nudge to see where you're at on this, @marshallward. Don't worry about it if you don't have the time, but I was curious if perhaps you'd changed your mind after seeing the PR diff (and maybe recoiling in horror 😄 )

[marshallward]

Thanks for the nudge, I looked it over and have started documenting the overall problem. I think I understand the approach but have been looking to see if it can be done with fewer (if any) metafields like _positional_row, but I haven't fully answered the question for myself.

I was planning to ping you because I tried this PR for the most recent issue #185, which I thought this would address, but didn't seem to work for me. Do you see the same?

[ken-lauer]

Hmm, reflecting on that issue and looking back over the changes after a couple months myself, it looks like I had some bad test cases that misrepresented how the scattered struct array/value data should look. Some of them definitely need fixing, and this PR definitely should not be merged as-is. I'll move it back to draft.

I'll poke around with this branch a bit more. I might also propose a different approach with a bit less bookkeeping metadata/metafields. Though it complicates things a bit, personally, I'd still like to see the namelists round-tripped to the same form (e.g., arr(1:N)%foo = v1, ..., vN doesn't become arr(1) = v1, arr(2) = v2, etc. or vice versa). Maybe a journal of sorts that tracks that information at the top-level of Namelist. 🤔

I'm betting you can come up with something cleaner eventually. That said, if what I propose ever gets to a point where you want to use it - great!

[marshallward]

I'll post where my thinking is up to on this. It duplicates some of what you and others have written but it will help me step through it.

The original sin is based on simple namelists like the following.

&grp
   v = 3, 4, 5
/

They have always been naively interpreted as arrays, but there are in fact two possiblities.

• Scalar array: v(:) = [3, 4, 5]
• Derived type:

  v%_1 = 3
  v%_2 = 4
  v%_3 = 5
  

  where _1, _2, _3 are unknown anonymized fields in v's derived type.

This first report of this problem appeared in #110 but it was incorrectly identified as an edge use case. Several issues later have shown otherwise.

In practice, the above example is not an issue. It is stored as nml['grp']['v'] = [3,4,5] and the RHS can be interpreted as list-directed input to be distributed across the records of v. It will be printed as v = 3, 4, 5.

Arrays of derived types as in #110 introduce new difficulties.

&grp
   v(1) = 3, 4, 5
   v(2) = 6, 7, 8
/

There are again two interpretations: v as a 2d array, or v as an array of derived types. However, I don't think the result is much different.

• v as a 2D array: v = [[3, 4, 5], [6, 7, 8]]
• v as a list of derived types with anonymous fields.

But we could just as easily interpret v as a list of list-directed input records. The real mistake was forcing the data into a 2d array. This was probably convenient for some people, but it was never necessary, and should have been held in a more generic form.

If you work up to higher dimensions, you see similar issues. For example:

&grp
   v%a = 3, 4, 5
/

Is a an array? a derived type? Or is v a list of scalars a? This is where #127 and #185 (and perhaps others) come in.

The one thing we can say for certain is that it is not v(:)%a(:), since multiple axes across types are not permitted within a namelist. But there is new ambiguity.

• v%a(:) and v%a, where a is either scalar array or derived type, are represented as

  nml['grp']['v'] = {'a': [3,4,5]}
  

  where the RHS is list-directed input.
• v(:)%a as a list of derived types with field a is represented as

  nml['grp']['v'] = [{'a': 3}, {'a': 4}, {'a': 5}]
  

The biggest question to me at the moment is how to capture this ambiguous state, rather than just defaulting to one or the other.

Unfortunately I don't have time to follow up at the moment, but the general point is that it would probably be better to rethink of the content as lists-of-data that are held on various containers. (I will try to follow up later tonight.)

[MuellerSeb]

This is great to see. I am really in need of this for my nml-tools package.
I think the issue with not being able to differentiate between arrays and buffer-style derived type assignment is not too bad as long as one can keep track of the buffer-style assignment itself, so f90nml doesn't add indices for derived type items by accident. (nml-tools actually tries to help with stuff like that, since it provides a schema definition for namelists, where one can infer the variable type from the schema).

Do you need any help here?

[ken-lauer]

I think we're all generally on the same page. Thanks for the great summary of the overall issue, @marshallward.

My original intent to get around that ambiguous scalar array vs anonymous derived type field issue was to keep track well enough to round-trip the data into the same representation. I don't think what this PR does is perfect in that regard, though. Things likely could be much simpler.

@MuellerSeb, I'm glad to hear there's more demand for this functionality. It's definitely something my team will be happy to have, once it makes its way into f90nml. As for help (at least on my end, @marshallward may have other ideas) if you could run this branch against your own test cases and such that would be useful. It may lead to either fixing this up or an entirely new approach.

[MuellerSeb]

For nml-tools I now implemented a schema aware parser built into nml-tools. This is much easier than guessing the underlying variable types from the present namelist itself like f90nml does (with good success BTW). But that is only usable to validate a namelist against a given schema and doesn't convert a namelist to a dictionary (yet).