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

[Bug]: wrong unit conversion in mean climate diagnostics #1128

Closed
zhangshixuan1987 opened this issue Sep 18, 2024 · 2 comments
Closed

[Bug]: wrong unit conversion in mean climate diagnostics #1128

zhangshixuan1987 opened this issue Sep 18, 2024 · 2 comments
Assignees
Labels

Comments

@zhangshixuan1987
Copy link
Collaborator

zhangshixuan1987 commented Sep 18, 2024

What happened?

The library of mean climate diagnostics includes a "load_and_regrid.py" module which attempts to extract the test and reference data at the pressure level. In the following lines:

below code block was added:

if "plev" in list(ds.coords.keys()):
            if ds.plev.units == "Pa":
                level = level * 100  # hPa to Pa
            try:
                ds = ds.sel(plev=level)

While in the ``mean_climate_driver.py" module, following
code block show up:

    result_dict["Variable"] = dict()
    result_dict["Variable"]["id"] = varname
    if level is not None:
        result_dict["Variable"]["level"] = level * 100  # hPa to Pa

This seems to be problematic, as you have converted the unit of level from hPa to Pa in mean_climate_driver.py, and then in the load_and_regrid.py, level times 100 again when the unit of plev in data file is "Pa". Usually, the unit convention for plev should be "Pa", although the unit of plev can be "hPa" or "mb" sometimes (depend on how user prepare the data).

Overall, I think that the unit conversion in the mean_climate_driver.py should be removed, and it is better to let the load_and_regrid.py deal with the unit conversion properly.

What did you expect to happen? Are there are possible answers you came across?

No response

Minimal Complete Verifiable Example (MVCE)

No response

Relevant log output

No response

Anything else we need to know?

No response

Environment

I am using the PCMDI master branch and run it on Argon Chryslis machine.

@lee1043
Copy link
Contributor

lee1043 commented Sep 21, 2024

Hi @zhangshixuan1987, thank you for the post. I see your point that it could be problematic particularly when the plev in the data is not [Pa]. As PMP has been developed with multi-model context, [Pa] for plev has been expected for the most of time as a result of CMOR.

The current code works as follows:

  1. in the parameter file level is expected to be given as hPa (e.g., 850)
  2. level [hPa] info is fed to load_and_regrid.py, but in the load_and_regrid.py, level is local variable that is not returned at the end of the function.
  3. So in the mean_climate_driver.py, level is still remain in [hPa] (even after level = level * 100 in load_and_regrid.py)
  4. result_dict["Variable"]["level"] = level * 100 # hPa to Pa is for storing the level info in [Pa] (that is in the data) into the output JSON file.

But as you pointed, above 4 can be problematic if ds.plev.units != "Pa" because then it is adding false info. To keep it simple, I like the change you proposed in #1132.

@lee1043
Copy link
Contributor

lee1043 commented Sep 21, 2024

Closing the issue as it was resolved by merging PR #1132.

@lee1043 lee1043 closed this as completed Sep 21, 2024
zhangshixuan1987 added a commit that referenced this issue Sep 23, 2024
related to #1128

In previous commit, changes in mean_climate_driver.py were made to
remove the forced unit conversion for pressure level. Accordingly,
the forced unit change in read_json_mean_clim.py should be removed
as well. Otherwise, the Metrics () fuction call to decode the metrics
json files will return wrong variable names.
zhangshixuan1987 added a commit that referenced this issue Sep 23, 2024
Modify read_json_mean_clim.py module to be consistent with the changes
related to #1128

In previous commit related to #1128,
changes in mean_climate_driver.py were made to remove the forced unit conversion
for pressure level. Accordingly, the forced unit change in read_json_mean_clim.py
should be removed as well. Otherwise, the Metrics () fuction to decode the metrics
json files will return wrong variable names.
zhangshixuan1987 added a commit that referenced this issue Sep 24, 2024
related to #1128

In previous commit, changes in mean_climate_driver.py were made to
remove the forced unit conversion for pressure level. Accordingly,
the forced unit change in read_json_mean_clim.py should be removed
as well. Otherwise, the Metrics () fuction call to decode the metrics
json files will return wrong variable names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants