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

Removed 'frac2percent' for all LPJG *Frac* variables #822

Closed
wants to merge 1 commit into from

Conversation

nierad
Copy link
Collaborator

@nierad nierad commented Apr 10, 2024

In the original LPJG-code the units for *Frac* variables has been changed from [fraction] to [%] and, therefore, the auto-conversion has been removed in lpjgpar.json accordingly.
The issue is related to https://dev.ec-earth.org/issues/1312#note-221
Thanks @treerink !

Copy link
Collaborator

@treerink treerink left a comment

Choose a reason for hiding this comment

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

I don't understand why there are changes in the cmip6-cmor-tables in the pull request. Anyway this changes shouldn't be there. Maybe better to create a branch with the intended changes for the lpjgpar.json and then I will take over the merging.

@klauswyser
Copy link
Collaborator

klauswyser commented Apr 11, 2024

My guess is that either @nierad didn't do the submodule update and thus the CMIP6 tables are out of sync, or that @nierad did update the submodule but the ece2cmor repo hasn't been updated lately. There was a recent update of the tables (addition of EC-Earth3-ESM-1) that will change the hash to the submodule in the ece2cmor repository. Anyway, I think it's just the hash but not any information in the table that will be affected by this pull request. (And of course it would be good if we can make sure that everything including hash for submodules is up to date).

But there is another issue with this PR: it breaks backward compatibility! The changed lpjgpar.json works fine with the OptimESM output, i.e. the output from the updated LPJG model. However, if anybody then uses the new ece2cmor to process data from a run with the old LPJG model (e.g. re-process CMIP6) then the results would be wrong. It would still work and no warning would be issued, and we will then just scratch our heads and don't understand why grassFrac is 5000% until somebody remembers #822. Can we avoid this? Having 2 versions of lpjgpar.json? Having a switch "new/old LPJG"? I don't have a good suggestion.

@nierad
Copy link
Collaborator Author

nierad commented Apr 11, 2024

@treerink : Well I am very confused. I thought I did exactly what you said I should do. No idea why the tables are changed. I only changed lpjgpar.json...
But anyways, re @klauswyser 's comment on the changes: We have had these kind of differences earlier, that is, we had to use a certain version for simulations done prior to a certain bugfix. That is not very cool indeed. Not sure what the best idea is either, but changing the code back to the original units isn't a big deal I guess. What do we do?

@treerink
Copy link
Collaborator

@treerink : Well I am very confused. I thought I did exactly what you said I should do. No idea why the tables are changed. I only changed lpjgpar.json

I prefer for ece2cmor3 that one opens an issue, prepares a branch and asks in the issue for merging. So don't do the pull request yourself (of course this often goes fine as well), but for instance not in this case.

... Not sure what the best idea is either, but changing the code back to the original units isn't a big deal I guess. What do we do?

Well for me is the easiest you set back the LPJG code and we keep the unit conversion in ece2cmor3 as we lately corrected that. Then the latest ece2cmor version works for all LPJG versions (for these variables). Using different lpjgpar.json technically works but doesn't help the ignorent user (or the busy one who overlooks this). Probably it is possible to make a check whether any point exceeds the value 100 and then one can decide which procedure (or warning) will be used if we trust that approach, but it is all not very beautiful. So from the ece2cmor3 perspective it is far the easiest that LPJG will be set back to its previous state (then the problem will only affect the current short-lived LPJG version).

@klauswyser
Copy link
Collaborator

So from the ece2cmor3 perspective it is far the easiest that LPJG will be set back to its previous state (then the problem will only affect the current short-lived LPJG version).

Not sure if that's the best thing to do. The LPJG developers have put a lot of effort in making the LPJG output "CMIP6 compatible" and I don't see the point in throwing away this effort. Furthermore, the updated LPJG version is the version that will be used in all future projects, not only with ECE4 but also with ECE3 and in particular ECE3-ESM. To my knowledge LPJG has been updated in the ECE3 trunk, not only in the OptimESM branch, so it's here to stay and w have to find a solution so cmorise output from the old and from the new LPJG.

@etiennesky
Copy link
Contributor

I agree it might be cleaner to revert the change in the lpjg code, so ece2cmor3 can work with both LPJG versions (the CMIP6 and post-CMIP6 one)

@klauswyser
Copy link
Collaborator

I agree it might be cleaner to revert the change in the lpjg code, so ece2cmor3 can work with both LPJG versions (the CMIP6 and post-CMIP6 one)

But then we would have different LPJG output for ECE3 and ECE4, do you really want that?

@etiennesky
Copy link
Contributor

Well then lpjg in ece4 would have to match ece3 as well for these variables.

@plesager
Copy link
Contributor

[...] We have had these kind of differences earlier, that is, we had to use a certain version for simulations done prior to a certain bugfix. That is not very cool indeed. Not sure what the best idea is either, but changing the code back to the original units isn't a big deal I guess. What do we do?

Sure you could revert LPJG. But do you want it? And a lot of years have been simulated already with that new version.

If you only need to change the lpjgpar.json file to process old experiments, the user can pass an older version of that file to the ece2cmor3 command. This is what I suggested to revert temporarily another "recipe-only" change, see: #772 (comment). This is better than modifying your default json file with a script (like switch-on-off-old-clear-sky-output.py). Indeed, the script solution is inconvenient, because:

  • all cmorizations are affected, which can be an issue when you cmorize various experiments in parallel (happens quite often with long running experiments)
  • you have to remember to rerun the script to switch off a change when you are done
  • it gets quickly confusing when you have several scripts (pextra, aerchem, foci, ...)

Note that a script is still useful to create the old json file (instead of overwriting the default one) and maintain it (stay consistent with other changes).

As for the EC-Earth3, processing output from codes based on 3.5.0 and above (which includes OptimESM) will need the new ec2cmor default. For all ECE versions before 3.5, including CMIP6, you will need to use the older json file. That's what users need to be aware of, and ec2mor should report in the log.

treerink added a commit that referenced this pull request Apr 11, 2024
…r LPJG versions (in case we go that way) see discussion in #822.
@treerink
Copy link
Collaborator

treerink commented Apr 11, 2024

Discussion has gone on while I just added a tiny script to the master: adjust-lpjgpar-for-previous-version.sh. This script and its name can be adjusted. It currently could add the conversion for all concerned variables as soon the new lpjgpar.json does not contain the conversion anymore. Currently this script will change the lpjgpar.json itself but it could make a copy instead of course. The reason to do it like this is to keep maintenance friendly, so if the lpjgpar.json changes in the future, just run this script and we are fine. Anyhow, what stays is that one has to know that for old LPJG versions you have take care of this. So that was what made me think set back LPJG for this conversion is maybe safer, but you can decide, I am fine with both solutions.

@treerink
Copy link
Collaborator

Anyway, I will block this pull request and remove the branch because if we want the conversion to be removed I will directly commit that changes in the master after just running sed -i '/frac2percent/d' lpjgpar.json (which gives identical results with the branch) and avoids the unintended cmor-tables changes.

@treerink treerink closed this Apr 11, 2024
@treerink treerink deleted the 821-remove_frac2percent_lpjg branch April 11, 2024 11:30
treerink added a commit that referenced this pull request Apr 11, 2024
… conversion after the LPJG code has been updated for this.
treerink added a commit that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversion from [frac.] to [%] in *Frac* variables no lnger needed in LPJ-GUESS
5 participants