Skip to content

Conversation

@davidhassell
Copy link
Contributor

Fixes #345

@davidhassell davidhassell added this to the NEXTVERSION milestone Jul 25, 2025
@davidhassell davidhassell added the enhancement New feature or request label Jul 25, 2025
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Good idea. It works and everything looks good implementation wise.

My main comment is that we might want to include the units in the data line, given for print(f) and f (i.e. str and repr) they don't show the data but do indicate the units, however one could argue the units are shown in the global properties so that's sufficient. Will leave it up to you.

>>> f
<Field: air_temperature(atmosphere_hybrid_height_coordinate(1), grid_latitude(10), grid_longitude(9)) K>
>>> print(f)
Field: air_temperature (ncvar%ta)
---------------------------------
Data            : air_temperature(atmosphere_hybrid_height_coordinate(1), grid_latitude(10), grid_longitude(9)) K
...
>>> f.dump(display=True, data=False)
---------------------------------
Field: air_temperature (ncvar%ta)
---------------------------------
Conventions = 'CF-1.12'
project = 'research'
standard_name = 'air_temperature'
units = 'K'

Data(atmosphere_hybrid_height_coordinate(1), grid_latitude(10), grid_longitude(9)) [add unit symbol here e.g. K as per the above reports?]
...

Also, ideally you can update the second-line docstring description for dump so that "... and provides selected values of all data arrays." is qualified with something like "... unless requested otherwise"/"optionally excluding the variable data" (at the end) given this new argument.

Please merge when considered.

davidhassell and others added 2 commits July 29, 2025 08:45
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@davidhassell
Copy link
Contributor Author

Hi Sadie,

we might want to include the units in the data line

That's a good idea: e42e09c

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the minor feedback/suggestion. Sanity check done on the new commits. All good, please merge.

@davidhassell davidhassell merged commit 875f7fe into NCAS-CMS:main Jul 31, 2025
@davidhassell davidhassell deleted the dump-data branch July 31, 2025 17:50
@davidhassell davidhassell modified the milestones: NEXTVERSION, 1.12.3.0 Oct 13, 2025
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.

Provide option to Field.dump to not display the first and last data values

2 participants