ecmwf / atlas

A library for numerical weather prediction and climate modelling
https://sites.ecmwf.int/docs/atlas
Apache License 2.0
115 stars 42 forks source link

field.rename does not update the fieldset index map #147

Closed jeromebarre closed 1 year ago

jeromebarre commented 1 year ago

What happened?

The method field.rename(<newname>) does not update the index of the fieldset, as seen in this code snippet: https://github.com/ecmwf/atlas/blob/0e00ad91bd8fdaaf1268619260c8488fa2626381/src/atlas/field/Field.h#L117

Instead, it only updates the metadata, as seen here: https://github.com/ecmwf/atlas/blob/0e00ad91bd8fdaaf1268619260c8488fa2626381/src/atlas/field/detail/FieldImpl.h#L102

This means that certain methods, such as fieldset.has(<newname>), become rather useless for renamed fieldsets since they check on index map and not the metadata. https://github.com/ecmwf/atlas/blob/0e00ad91bd8fdaaf1268619260c8488fa2626381/src/atlas/field/FieldSet.cc#L44-L46 The method fieldset.field_names() returns the correct changed names as I think it uses the metadata.

What are the steps to reproduce the bug?

create a filedset rename the fieldset: fieldset.field(old_name).rename(new_name) try using fieldset.field_names() : this returns [new_name] try using fieldset.has(new_name) try using fieldset.has(old_name)

Version

ecmwf-atlas-0.32.1

Platform (OS and architecture)

MacOS 12.6.2

Relevant log output

No response

Accompanying data

No response

Organisation

UCAR/JCSDA

wdeconinck commented 1 year ago

Hi @jeromebarre, this is indeed an inconsistency. A probable fix for this would be to register Field observers in a FieldSet, so that when a Field modifies its name it triggers in the fieldset to update its mapping. I had thought about this in the past but never thought there would be an actual use case for this. I'll put this in my backlog for implementation when I find some time.

wdeconinck commented 1 year ago

The fix for this has been merged in develop now

jeromebarre commented 1 year ago

This is great thanks.