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

Remove derived variable gtfgco2 #418

Merged
merged 1 commit into from
Jan 9, 2020
Merged

Remove derived variable gtfgco2 #418

merged 1 commit into from
Jan 9, 2020

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Jan 3, 2020

Because it can be computed from fgco2 with the area_statistics preprocessor, see #83 (comment)

Before you start, please read CONTRIBUTING.md.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • If writing a new/modified preprocessor function, please update the documentation -> I don't think this is documented anywhere
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below -> The only recipe where this derived variable is mentioned is recipe_ocean_bgc.yml, but it is commented out.

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Partially addresses #42

Because it can be computed from fgco2 with the area_statistics preprocessor
@bouweandela bouweandela merged commit 6475164 into master Jan 9, 2020
@bouweandela bouweandela deleted the remove_gtfgco2 branch January 9, 2020 09:33
@ledm
Copy link
Contributor

ledm commented May 28, 2020

I'm just in the process of preparing a PR for the GMD paper (ESMValGroup/ESMValTool#1621) and I need to re-open this issue (sorry). I realise that I'm a bit late to the party here, but it's still not strictly possible to fully replace this derived variable with a preprocessor at the moment. (Similarly for the global total integrated primary production derived variable, which is not mentioned here but also needed for this publication.)

The area_statistics preprocessor sum operator multiplies and sums the data and the area, and this process change the units. In this case, the units change from from kg m-2 s-1 in fgco2 to kg s-1 for the global total dervied variable gtfgco2, which is then converted into Pg/year.

Even now, it's not clear that there's a solution where preprocessors can change the dimensions of the output units. @valeriupredoi like to bring up Iris's cube.convert_units(new_units) function, but this function will be able to change the dimensionality of the units, and it is not sensible for it to do so either. Also the preprocessed version may only use the units listed in the CMOR tables, but the derived output netcdf can have any units.

Can we annul this PR and re-instate the global total fgco2?

@ledm
Copy link
Contributor

ledm commented May 28, 2020

Instead of annulling this PR, is there a solution where area_statistics sum can change the cube's units?

@valeriupredoi
Copy link
Contributor

no, I don't think it's a good idea to revert this PR. Instead, maybe you can create a preprocessor that calculates a flux like in the derivation script that got deleted but rather using dask arrays for laziness (that may be useful for other variables and being a preprocessing function preserves a good degree of generality)? 🍺

@ledm
Copy link
Contributor

ledm commented May 29, 2020

@valeriupredoi, I think that you're missing the point here. The area_statistics sum operation calculates the area-weighted sum. This means that it multiplies the value, either a concentration [per cubic meter] or a flux [per square meter] by the area [square meters]. This changes the units, which isn't reflected by the preprocessor at the moment.

This means that every preprocessor that uses the area_statistics sum operation with fx files is scientifically incorrect. However, if you attempt to change the units in a preprocessor, the netCDF is no longer CMOR compliant. This is why we needed a global total air sea flux of CO2 derived variable.

@bouweandela
Copy link
Member Author

@ledm Changing the units shouldn't be that big of a problem, so long as you do it after the cmor checks are performed, so I guess you could work around issues #212 and #505 by using a custom order for now. Would that be an option? See also discussion in #212.

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

Successfully merging this pull request may close these issues.

4 participants