Units reported in metadata.yml may differ from actual units

[bouweandela]

Because the units reported in metadata.yml are read from the CMOR table, preprocessing operations that change the units of the data will make this information invalid.

Maybe we could just remove the units entry from metadata.yml/varname_info.ncl files, or we should read it from the cube before saving.

[bouweandela]

@mattiarighi @schlunma Any idea if diagnostics actually use the units attribute from the interface instead of from the data?

[schlunma]

@mattiarighi @schlunma Any idea if diagnostics actually use the units attribute from the interface instead of from the data?

I have several examples where I check that both are equal (so it would be fine to just use once). I think it would be nice to drop the attributes in metadata.yml, but we need to check a lot of diagnostics for that.

Alternatively, we could set the attribute to cube.units in last preprocessor step (assuming that cube.units was modified correctly by the preprocessors).

[mattiarighi]

I tried to grep quickly through the NCL scripts and there are a lot of occurrence of @units. My feeling is that this is read from the data, but it's hard to tell without looking at every script in detail.

Checking all the affected diags might be lot of work, it could be more effective:

• to remove the units entry from metadata.yml/varname_info.ncl as suggested
• to run the affected recipes / diagnostics
• to fix the errors which may occur

Such a change would require testing all the affected recipes anyway.

[schlunma]

Note that this might also apply to other attributes such as short_name, long_name, etc. as discussed in #759. A possible solution is given here.

[bouweandela]

@bettina-gier Since you reported this issue again in #866, do you have an opinion on if having units in metadata.yml/var_info.ncl is a useful feature? I have a slight preference for just removing it, because the information is already available in the data.

[bettina-gier]

In ncl it's nice because a lot of diagnostics set up their arrays to save data in before loading it, and then they can already set the units in the same spot. But it's not a problem to do it later and probably better to read it from the data as that is more likely to be correct in the current state.
So unless we make sure that all preprocessor functions would reflect correct units and add this to the units attribute in those files, it's better to remove them to not give misleading (or plain wrong) information.

[schlunma]

@bouweandela is the underlying code that needs to be changed here somehow modified in #1609? I'd like to work on this since it's bothering me a lot lately, but don't want to create horrible merge conflicts for you.

[bouweandela]

A bit but it shouldn't be a problem (you can see the changes here: https://github.com/ESMValGroup/ESMValCore/pull/1609/files#diff-4c5468e8eebd3bebe05236ed962fa98483d27a9374d17c0ad43ba19287435a5d). You'll probably want to update the PreprocessorFile.save method so it updates PreprocessorFile.attributes with the correct facet values. That's probably more than just the units: it could also be standard_name, long_name, and frequency.