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 fix in module mean_climate_driver.py for regional mean #1133

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

zhangshixuan1987
Copy link
Collaborator

@zhangshixuan1987 zhangshixuan1987 commented Sep 22, 2024

Changes are made to mean_climate_driver.py to call the updated version of region_subset() function. Specifically, current mean_climate_driver.py passed three input parameters in to region_subset() to mask the data for regional mean climate calculation. However, the region_subset() in pcmdi_metrics.io library required 5 input parameters, leading to the failure of mean_climate_driver.py calculation.

The reason for the problems in mean_climate_driver.py is due to the update of the region_subset() function:

  • For the old version of mean_climate_driver.py, region_subset() was included in default_regions_define.py as shown below:
def region_subset(ds, regions_specs, region=None):
    """
    d: xarray.Dataset
    regions_specs: dict
    region: string
    """

Where the three input parameters ds, regions_specs, and region are used to mask the input ds data array.

  • In current version of mean_climate_driver.py, region_subset() was moved to regions.py and update to the following formula:
def region_subset(
    ds: Union[xr.Dataset, xr.DataArray],
    region: str,
    data_var: str = "variable",
    regions_specs: dict = None,
    debug: bool = False,
)

Where the input parameters and position are changed. However, associated changes in mean_climate_driver.py were not made accordingly to feel the change in regional_subset function.

In this commit, I made corrections in mean_climate_driver.py to use the new region_subset() function as mentioned above. A special note for my changes, see example below:

ds_test_tmp = region_subset(
    ds_test_tmp,
    region,
    None,
    regions_specs,
)

the "None" is always used for the variable name as I think this would be more general as it is not clear whether "varname" or "varname_in_file" should be passed to this function.

zhangshixuan1987 and others added 8 commits September 22, 2024 12:38
Changes are made to mean_climate_driver.py to call the updated version
of region_subset() function. Specifically, current mean_climate_driver.py
passed three input parameters in to region_subset() to mask the data for
regional mean climate calculation. However, the region_subset() in
pcmdi_metrics.io library required 5 input parameters, leading to the
failure of mean_climate_driver.py calculation.
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.
Changes are made to mean_climate_driver.py to call the updated version
of region_subset() function. Specifically, current mean_climate_driver.py
passed three input parameters in to region_subset() to mask the data for
regional mean climate calculation. However, the region_subset() in
pcmdi_metrics.io library required 5 input parameters, leading to the
failure of mean_climate_driver.py calculation.
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.
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.
Remove forced unit conversion in read_json_mean_clim.py
@lee1043
Copy link
Contributor

lee1043 commented Sep 23, 2024

Instead of passing None, default for the parameter data_var is set to None in the function level. Also updated docstrings for the function. Parameter order matched and clarified by explicitly assigning.

@zhangshixuan1987
Copy link
Collaborator Author

@lee1043 : Hi Jiwoo, you modifications look more clearer to me.

@lee1043
Copy link
Contributor

lee1043 commented Sep 24, 2024

Confirmed that no error introduced to demo 1a and 1b notebooks.

lee1043 and others added 7 commits September 23, 2024 20:01
Current compute_metrics.py used either ".5f" or ".3f" to form the
floating numbers for the output from the mean climate driver. As the
magnitude of the model variable can range from 1e5 (e.g pressure ) to
1e-3 (e.g. humidity), used the ".5f" and ".3f" can sometimes truncate
the small values to zero. It seems that using  a standard scientific
notation can be benificial in case the user processed the data without
converting the unit data ahead. In this way, it also provide
possibilities for users to convert the units within the diagnostic
datasets.
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.

2 participants