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

Simplify pr variability driver #828

Merged
merged 8 commits into from
Jan 13, 2022

Conversation

lee1043
Copy link
Contributor

@lee1043 lee1043 commented Jan 7, 2022

In this PR I've simplified the pr variability driver by modularizing metric calculation part, in order to easily call from precip benchmarking metrics collective driver.

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 7, 2022

I have verified that the demo 7 results (statistics numbers in output JSON) is not affected and remaining identical.

@lee1043 lee1043 requested a review from msahn January 7, 2022 00:30
@lee1043 lee1043 self-assigned this Jan 7, 2022
@msahn
Copy link
Collaborator

msahn commented Jan 10, 2022

I tried to test this code on Gates by cloning this branch, and I found there is an issue to call 'pcmdi_metrics.precip_variability.lib' as below.

Traceback (most recent call last):
  File "/home/ahn6/PCMDI/branch/lee1043_simplify_pr_variability_driver/pcmdi_metrics/pcmdi_metrics/precip_variability/scripts_pcmdi/../variability_across_timescales_PS_driver.py", line 10, in <module>
    from pcmdi_metrics.precip_variability.lib import (
ModuleNotFoundError: No module named 'pcmdi_metrics.precip_variability'

I think this issue occurs after updating "pcmdi_metrics" to version 2.1.2. Could you check this issue?

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 10, 2022

@msahn thanks for testing the revised scripts. Does that issue occurs with version 2.2.1 as well?

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 11, 2022

@msahn could you try import pcmdi_metrics and share the result? Just curious if missing module is only precip_variability or others as well.

@msahn
Copy link
Collaborator

msahn commented Jan 11, 2022

@msahn thanks for testing the revised scripts. Does that issue occurs with version 2.2.1 as well?

I have tested it with pmp v2.2.1, and it is still not working, but the error message is changed. Below is the error message. It seems that "precip_variability_across_timescale" is not added in the file "/home/ahn6/anaconda3/envs/pmp_v20220110/lib/python3.9/site-packages/pcmdi_metrics/precip_variability/lib/init.py".

"log_PS_3hr_TRMM" 4L, 524C                                                                                                            4,1           All
Traceback (most recent call last):
  File "/home/ahn6/PCMDI/branch/lee1043_simplify_pr_variability_driver/pcmdi_metrics/pcmdi_metrics/precip_variability/scripts_pcmdi/../variability_across_timescales_PS_driver.py", line 10, in <module>
    from pcmdi_metrics.precip_variability.lib import (
ImportError: cannot import name 'precip_variability_across_timescale' from 'pcmdi_metrics.precip_variability.lib' (/home/ahn6/anaconda3/envs/pmp_v20220110/lib/python3.9/site-packages/pcmdi_metrics/precip_variability/lib/__init__.py)

@msahn
Copy link
Collaborator

msahn commented Jan 11, 2022

@msahn could you try import pcmdi_metrics and share the result? Just curious if missing module is only precip_variability or others as well.

import pcmdi_metrics is working well with pmp v2.2.1.

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 12, 2022

@msahn helped identifying where the issue was coming from. It looks like once pmp installed through conda-forge, re-installing from cloned repo is not overwriting the pre-installed pmp. One work around is to generate development dedicated conda env (with all dependecies installed but pmp), following here, then install pmp from the cloned repo, to test the branch in development.

@msahn, once you confirm that driver is working correctly in your development env, could you please approve your review, so I can merge following the main branch protection protocol? (at least one review required for merging) Thanks!

@msahn
Copy link
Collaborator

msahn commented Jan 13, 2022

@msahn helped identifying where the issue was coming from. It looks like once pmp installed through conda-forge, re-installing from cloned repo is not overwriting the pre-installed pmp. One work around is to generate development dedicated conda env (with all dependecies installed but pmp), following here, then install pmp from the cloned repo, to test the branch in development.

@msahn, once you confirm that driver is working correctly in your development env, could you please approve your review, so I can merge following the main branch protection protocol? (at least one review required for merging) Thanks!

I have tested this code in my pmp development env and confirmed that the results are identical to the previous version.

@lee1043 lee1043 merged commit 1cbb652 into PCMDI:main Jan 13, 2022
@lee1043 lee1043 deleted the simplify_pr_variability_driver branch January 13, 2022 01:19
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.

2 participants