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

Adding support for new srng (slant range) field in grid and map files #75

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

egthomas
Copy link
Member

@egthomas egthomas commented Aug 5, 2024

Scope

This pull request adds support for a new (optional) vector.srng field being added to grid and map-format dmap files in the RST (SuperDARN/rst#616).

Approval

Number of approvals: 1 (?)

Test

To test, try creating a grid file using make_grid with the RST's feature/grid_slant_range branch, and then try opening / reading the grid file with this branch. Or I can provide a test file if needed.

@egthomas egthomas added the enhancement New feature or request label Aug 5, 2024
Copy link
Collaborator

@carleyjmartin carleyjmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me a while to get to this, I've also been having the generic Mac and RST install issues and was reluctant to install a new branch (but it all worked out so yay!).

After fixing the two syntax errors (suggestions), there seems to be a larger bug (which has nothing to do with this PR, and I suspect has been around since the beginning in the exceptions) where we haven't dealt with optional fields and extra fields together before, and it's not actually doing what we expect it to do with optional fields when doing the extra field check - I will look into it and see what needs doing to fix.

The traceback for my own records is:

Error: The following fields in record 0 are not allowed: {'vector.srng'}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/carley/Documents/venvs/venv-pydarnio-srng/lib/python3.12/site-packages/pydarnio/dmap/superdarn.py", line 515, in read_grid
    self._read_darn_records(file_struct_list, optional_list)
  File "/Users/carley/Documents/venvs/venv-pydarnio-srng/lib/python3.12/site-packages/pydarnio/dmap/superdarn.py", line 397, in _read_darn_records
    self._read_darn_record(format_fields, optional_list)
  File "/Users/carley/Documents/venvs/venv-pydarnio-srng/lib/python3.12/site-packages/pydarnio/dmap/superdarn.py", line 368, in _read_darn_record
    SDarnUtilities.extra_field_check(format_fields, record, self.rec_num)
  File "/Users/carley/Documents/venvs/venv-pydarnio-srng/lib/python3.12/site-packages/pydarnio/dmap/superdarn.py", line 212, in extra_field_check
    raise superdarn_exceptions.SuperDARNExtraFieldError(rec_num,
pydarnio.exceptions.superdarn_exceptions.SuperDARNExtraFieldError: Error: The following fields in record 0 are not allowed: {'vector.srng'}

I also added the line optional_list = [superdarn_formats.Grid.optional_fields] on L514 and passed optional_list to the read darn records method on the line below. This still didn't make it work, but it needs adding anyway I think (and this will need doing for read map too).

pydarnio/dmap/superdarn_formats.py Outdated Show resolved Hide resolved
pydarnio/dmap/superdarn_formats.py Outdated Show resolved Hide resolved
@egthomas
Copy link
Member Author

Thanks @carleyjmartin! And apologies for not actually testing my own proposed changes - I'm very much a novice on the pydarnio side of things.

Is there a practical difference between "optional" and "extra" fields? My assumption was that "optional" is meant for fields that have been added in subsequent file format updates (eg fitting algorithm or tdiff in fitacf files), whereas "extra" is meant for an "all or nothing" set of fields that may or may not be included (eg power, width in grid and map files)?

@carleyjmartin
Copy link
Collaborator

As far as I'm aware you are correct, the extra fields are a set that always come together or none of them are there, and the optional fields can come and go as they please. But practically, I don't really think they're all that different, does pydarnio really need to error out if there power but no width?

@carleyjmartin
Copy link
Collaborator

I made the updates needed anyway to get everything working (without doing any philosophical changes to why things are done, but we should have a chat about this at the next DVWG meeting anyway). The write functions were already amended, so I wonder how we missed the read functions.

If someone else (Rem? 👀 ) could test and check that the map, grid and fitacf stuff is reading and/or writing without issues. With the new optional field or not. Below is a zip file of a single grid file made with the new field in to test if you can't or don't want to install the new RST branch.
20211102.0000.00.rkn.b.newgridversion.grid.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants