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

Lists of namelists cannot be added #167

Open
rhfogh opened this issue Nov 26, 2024 · 4 comments
Open

Lists of namelists cannot be added #167

rhfogh opened this issue Nov 26, 2024 · 4 comments

Comments

@rhfogh
Copy link

rhfogh commented Nov 26, 2024

A namelist containing a list of namelists created in Python cannot be written to file and throws an error.

The test program:

import f90nml
data = f90nml.Namelist()
data["dict1"] = f90nml.Namelist(attr1=1, attr2=2)
data["list1"] = []
data["list1"].append(f90nml.Namelist(attr3=3, attr4=4))
data["list1"].append(f90nml.Namelist(attr3=5, attr4=6))
data.write("somefile.out")

gives - for Python 3.8 and f90nml versions 1.4.1, 1.4.3, and 1.4.4

Traceback (most recent call last):
File "/home/rhfogh/Software/scripts/test_f90nml.py", line 9, in
data.write("/home/rhfogh/calc/mxcube_data/f90nmltst5.out")
File "/alt/rhfogh/Software/miniconda3/envs/mxcubeqt/lib/python3.8/site-packages/f90nml/namelist.py", line 578, in write
self._writestream(nml_file, sort)
File "/alt/rhfogh/Software/miniconda3/envs/mxcubeqt/lib/python3.8/site-packages/f90nml/namelist.py", line 677, in _writestream
self._write_nmlgrp(grp_name, grp_vars, nml_file, sort)
File "/alt/rhfogh/Software/miniconda3/envs/mxcubeqt/lib/python3.8/site-packages/f90nml/namelist.py", line 693, in _write_nmlgrp
for v_name, v_val in grp_vars.items():
AttributeError: 'list' object has no attribute 'items'

With the output:

&dict1
attr1 = 1
attr2 = 2
/

&list1

When running with Python 3.8 and f90nml version 1.3.1 the test works and you get the desired result:

&dict1
attr1 = 1
attr2 = 2
/

&list1
attr3 = 3
attr4 = 4
/

&list1
attr3 = 5
attr4 = 6
/

@marshallward
Copy link
Owner

I think that this line is problematic: data['list1'] = [].

f90nml 1.4 did a lot of work to improve support for namelist groups with the same name, but it also forced the interface to become more complicated. Repeated namelist groups now must be declared as "Cogroups" rather than treating them as lists.

This would be the new way to set up your namelist:

import f90nml

data = f90nml.Namelist()

data['dict1'] = f90nml.Namelist(attr1=1, attr2=2)

data.create_cogroup('list1')  # This is optional

data.add_cogroup('list1', f90nml.Namelist(attr3=3, attr4=4))
data.add_cogroup('list1', f90nml.Namelist(attr5=5, attr6=6))

I did try to preserve the existing behavior when accessing the Namelist (e.g. Namelist['list1'][0] should return the first one), but did not consider the construction of the Namelist with explicit lists. Maybe there is some way to restore this old behavior.

@marshallward
Copy link
Owner

Assuming there are no unexpected issues, I think we could modify things so that data['list1'] = [] is equivalent to data.create_cogroup('list1') and the data['list1'].append() calls are equivalent to data.add_cogroup('list1', ...).

@rhfogh
Copy link
Author

rhfogh commented Nov 26, 2024

@marshallward That would be great. Now that I understand how it ought to work (thanks!) I could also rewrite our code to use the cogroups, but it would be kind of neat that you can use the standard Python datatype, a list.

One question springs to mind, though: What should happen if you did data.create_cogroup('list1') followed by a single data.add_cogroup('list1', ...) call, or even none? Or, equivalently, if you added an empty list or a list with a single member? In normal Python logic you would expect the result to be an empty list, or a list with one member as appropriate, but with Fortran logic you might expect something else. If you cannot get the Python-expected behaviour from adding Python lists without confusing the Fortran expectations, it might be better to force people to use the cogroup syntax after all; that way there would be fewer surprises. If that is the preferred solution I would vote for a better error message, though, something about adding an illegal type.

PS I do not really know enough about f90nml, but I am assuming that when reading data back, data['list1'] would return a Python list - or something that behaved like it. If not, the ball game would change .

@marshallward
Copy link
Owner

Cogroup was originally meant to be an internal type that was invisible to the user, but I did eventually decide to create a small API during testing. It is a concise way to set them up, but it does add more concepts for the user to absorb.

data.create_cogroup('list1') sets up the metadata to track list1 as a cogroup. If there are no list1 groups, then data['list1'] returns an empty list. But if you iterate over values of data, then data will appear empty, with no reference to list1.

If list1 is declared to be a cogroup, then data['list1'] returns a list of Namelist values. If there's one list1 group, then it's a list of one. I can't recall if this is different from v1.3. Iteration will return the group, not the list.

In Fortran the namelist would be read serially, so data['list1'] would probably simply return the first group in the list, with subsequent reads giving the next in the list, and so forth, which I don't think anyone wants. I suppose we are trying to build a datatype that exists inbetween the Fortran and Python environments.

I think there is certainly a lot of nuance here, which you are doing a great job of exposing. My goal at the time was to support while preserving the old API for reading. It might be worth writing up a small document about Cogroups at some point.

Thanks very much for your feedback! I confess that I don't think much about namelist generation within Python. Unsurprisingly, I think most of the bug reports are from users who do this!

I would like to keep this issue open for a bit longer, while I test out list assignment to Namelist. If it doesn't affect the other uses of Namelist (or if I finally decide to split it into Namelist, Group, DerivedType), then I think it would work nicely.

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

No branches or pull requests

2 participants