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

Precip distribution #861

Merged
merged 47 commits into from
Oct 15, 2022
Merged

Precip distribution #861

merged 47 commits into from
Oct 15, 2022

Conversation

msahn
Copy link
Collaborator

@msahn msahn commented Jul 18, 2022

This is to implement precip distribution metrics into the PMP.

@msahn msahn marked this pull request as draft July 18, 2022 23:03
@msahn
Copy link
Collaborator Author

msahn commented Jul 18, 2022

@lee1043 @acordonez
Could you help review this pull request when you have time? It is not urgent because this branch should be merged right before our paper is submitted.

I have a question. In the JSON output structure (see below), there is a hierarchical structure for seasons and months. Is this structure working for CMEC as well? or Should I update it as a flat structure?

image

@msahn msahn requested review from lee1043 and acordonez July 18, 2022 23:25
@acordonez
Copy link
Collaborator

@msahn I'll review this by the end of the week. Can I run these metrics with data that is already in the PMP sample data set?

Copy link
Collaborator

@acordonez acordonez left a comment

Choose a reason for hiding this comment

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

@msahn Can you add your driver script(s) to setup.py here?

@msahn
Copy link
Collaborator Author

msahn commented Jul 19, 2022

@msahn Can you add your driver script(s) to setup.py here?

Thank you for pointing it out. I have added my driver script to setup.py.

@msahn
Copy link
Collaborator Author

msahn commented Jul 19, 2022

@msahn I'll review this by the end of the week. Can I run these metrics with data that is already in the PMP sample data set?

Thank you. Yes, it works with daily precipitation data. The default reference data for the Perkins score and other metrics that need reference data is set as IMERG as below. So, this should be also changed to your input data for testing.

@acordonez
Copy link
Collaborator

@lee1043 @acordonez Could you help review this pull request when you have time? It is not urgent because this branch should be merged right before our paper is submitted.

I have a question. In the JSON output structure (see below), there is a hierarchical structure for seasons and months. Is this structure working for CMEC as well? or Should I update it as a flat structure?

image

@msahn This structure works fine in the CMEC JSONs.

import glob
import copy
import pcmdi_metrics
import regionmask
Copy link
Collaborator

Choose a reason for hiding this comment

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

regionmask and rasterio are not currently dependencies for the PMP. Are there any other packages, along with these packages, that need to be added as dependencies in conda-forge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How can I check the current dependencies for the PMP? I think the packages for defining regions below need to be additionally added.

  • regionmask
  • rasterio
  • shapely

Copy link
Collaborator

@acordonez acordonez Jul 20, 2022

Choose a reason for hiding this comment

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

The dependencies for conda-forge are defined here: https://github.com/conda-forge/pcmdi_metrics-feedstock/blob/main/recipe/meta.yaml#L30

While shapely is not explicitly defined, it is already being installed in the pcmdi_metrics environment as a dependency for the defined packages. Rasterio and regionmask are not. If you want to see everything that gets installed by default, you can create a pmp environment and use conda list like this:

conda create -n new_pmp_env -c conda-forge pcmdi_metrics
conda activate new_pmp_env
conda list

When I was testing the IMERG job, I also found that 'netcdf4' needed to be explicitly installed or I'd get an error in xarray. So in total I think we'd need to add 'rasterio', 'regionmask', and 'netcdf4' to meta.yaml.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the information. Adding the three packages sounds good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@acordonez Are those libraries needs to be added to conda-env/dev.yml file as well? Sorry for my delayed response on this thread.

pcmdi_metrics/precip_distribution/README.md Show resolved Hide resolved
@lee1043 lee1043 marked this pull request as ready for review October 13, 2022 00:26
@msahn
Copy link
Collaborator Author

msahn commented Oct 13, 2022

I have removed the unused local variables in "lib_precip_distribution.py" but could not fix the issues in "parallel_driver_cmip5.py" and "parallel_driver_cmip6.py". In the codes, "modpath" and "res" are defined from parameter files, but it seems like the checking algorithm can not recognize it.

@msahn I merged parallel_driver_cmip5.py and parallel_driver_cmip6.py into a single file, parallel_driver_cmip.py, which reads in the parameter file.

python -u parallel_driver_cmip.py -p ../param/precip_distribution_params_cmip5.py

Its usage for cmip5 and cmip6 is briefly documented here. Could you check if this is okay and work for you?

I also wonder if this setup allows running the code for just one or a few models. If so could you explain how?

Thank you for updating the code to be more efficient. I have tested it, and there was no issue. I also have added functionality in parallel_driver_cmip.py for running the code for one model using --mod option.

@lee1043 lee1043 linked an issue Oct 14, 2022 that may be closed by this pull request
@lee1043
Copy link
Contributor

lee1043 commented Oct 14, 2022

After adding dependencies in dev.yml github action's build test get automatically cancelled every time it was triggered.

@acordonez do you have any idea on this? Should we consider specifying versions of the newly added dependencies?

@lee1043
Copy link
Contributor

lee1043 commented Oct 14, 2022

Note: Below four dependencies were added in conda-env/dev.yml. I think I can include them as default for now.

  • netcdf4=1.6.0
  • regionmask=0.9.0
  • rasterio=1.2.10
  • shapely=1.8.0

However, considering they are only used for precip distribution metric, further discussion will be needed to decide whether we want to keeping them as default or optional.

@acordonez @msahn any comment welcome.

@acordonez
Copy link
Collaborator

@lee1043 We'll also need to add those as dependencies in the conda feedstock meta.yaml if we want them to be installed with PMP via conda-forge. I've had a lot of trouble with shapely in particular trying to install it after creating a new environment with other packages in it, so installing these at the same time as the other PMP dependencies is probably the best way to go.

@lee1043
Copy link
Contributor

lee1043 commented Oct 14, 2022

@lee1043 We'll also need to add those as dependencies in the conda feedstock meta.yaml if we want them to be installed with PMP via conda-forge. I've had a lot of trouble with shapely in particular trying to install it after creating a new environment with other packages in it, so installing these at the same time as the other PMP dependencies is probably the best way to go.

@acordonez good to know! Agreed, thanks for your input!

@acordonez
Copy link
Collaborator

@lee1043 @msahn Looking at the build failures...I don't see anything wrong, but I'll keep looking.

@acordonez
Copy link
Collaborator

@lee1043 @msahn I'm stumped about this failure. I've tried rerunning the job a couple of times and it happens in the same step each time. I don't see any warnings or messages related to the new dependencies, but maybe there's some additional step we have to take to include them. Do you think we could ask Jason or Tom?

@lee1043
Copy link
Contributor

lee1043 commented Oct 15, 2022

@acordonez thanks for checking. Same here, I have looked through messages, rerunned build test multiple times with enabling debugging, but it keeps cancels after ~5 min for no obvious region, which is strange. Do you think it is okay we just override it for now...?

@lee1043
Copy link
Contributor

lee1043 commented Oct 15, 2022

By above 2 commits, I confirmed that the build test cancelling was not influenced by newly added dependencies in the dev.yml. With that it may be okay to override...

Copy link
Contributor

@lee1043 lee1043 left a comment

Choose a reason for hiding this comment

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

I have confirmed the code works well on my end. Thanks @msahn for the contribution!

@lee1043 lee1043 merged commit 35107eb into main Oct 15, 2022
@lee1043 lee1043 deleted the 763_msa_precip_distribution branch October 15, 2022 03:33
@lee1043 lee1043 changed the title 763 msa precip distribution Precip distribution Oct 15, 2022
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.

precip distributions
3 participants