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.

Sign up for GitHub

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

Jump to bottom

CDAT Migration Phase 2: Refactor streamflow set #837

Merged
merged 15 commits into from
Aug 26, 2024
Merged

CDAT Migration Phase 2: Refactor streamflow set #837

merged 15 commits into from
Aug 26, 2024

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Aug 22, 2024 •
edited
Loading

Description

TODO

08/22/24

  • Refactor driver
    • run_diag()
    • setup_test()
      • debugging_case_setup_test() -- deleted, conditional for this case is never True
        • get_e3sm_flow()
        • get_area_upstream()
        • get_test_array()
    • setup_ref()
    • generate_export()
    • get_drainage_area()
    • get_seasonality()
  • Refactor plotter
    • streamflow_plot_seasonality.py -- in progress
      • plot_seasonality_map()
      • plot_panel_seasonality_map()
    • streamflow_plot_map.py
      • plot_annual_map()
        • setup_annual_map()
        • plot_gauges_annual_map()
    • streamflow_plot_scatter.py
      • plot_annual_scatter()

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@tomvothecoder tomvothecoder added the cdat-migration-fy24 CDAT Migration FY24 Task label Aug 22, 2024
@tomvothecoder tomvothecoder self-assigned this Aug 22, 2024
@chengzhuzhang
Copy link
Contributor

@tomvothecoder I just saw that you are starting a PR for streamflow analysis. Great progress! I think the logic of the original code base can perhaps be simplified. Also there was an issue with this set, even when model vs model case, the set needs additional data from observation data. The issue is documented here. #588

I think it might be a good opportunity to re-write this set with the issue also resolved. But I think i will let you to decide how best to approach this...

@tomvothecoder
Refactor _run_diags() and child functions
71c6f93 
- Extract `_get_gauges()` function
- Rename `setup_test()` to `_get_test_data_and_area_upstream()`
- Renme `setup_ref()` to `_get_ref_data()`
- Extract `_get_var_data()` function
- Start refactoring `_generate_export()` function
@tomvothecoder
Start refactoring plotters
c306d9e
@tomvothecoder
Split up streamflow plots into separate modules
c2f9e2d
@tomvothecoder
Clean up seasonality plot;
b4d59b5
@tomvothecoder
Refactor streamflow_plot_map
809bfb3
@tomvothecoder
Fix incorrect sizing of seasonality map markers
f743c16
@tomvothecoder
Various code cleanup changes
d7d056c 
- Add docstrings and type annotations
- Reorder function arguments
- Remove unnecessary exception raises
@tomvothecoder
Clean up driver
ddf0d26
@tomvothecoder
Update docstring for plotter
aecf482
@tomvothecoder tomvothecoder marked this pull request as ready for review August 26, 2024 19:38
@tomvothecoder
Copy link
Collaborator Author

Dev plots: https://portal.nersc.gov/project/e3sm/cdat-migration-fy24/665-streamflow/viewer/streamflow/index.html
Main plots: https://portal.nersc.gov/project/e3sm/cdat-migration-fy24/main/viewer/streamflow/index.html

Regression testing result -- all three plots are identical

tomvothecoder
tomvothecoder commented Aug 26, 2024
View reviewed changes
Copy link
Collaborator Author

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

Final self-review

e3sm_diags/plot/streamflow_plot_map.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_map.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_map.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_map.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_seasonality.py Outdated Show resolved Hide resolved
tomvothecoder
tomvothecoder commented Aug 26, 2024
View reviewed changes
Copy link
Collaborator Author

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

Final final self-review

e3sm_diags/plot/streamflow_plot_scatter.py Outdated Show resolved Hide resolved
e3sm_diags/plot/streamflow_plot_seasonality.py Outdated Show resolved Hide resolved
@tomvothecoder tomvothecoder merged commit 6f889f8 into cdat-migration-fy24 Aug 26, 2024
4 checks passed
@tomvothecoder tomvothecoder deleted the refactor/665-streamflow branch August 26, 2024 20:28
@tomvothecoder tomvothecoder mentioned this pull request Aug 26, 2024
Closed
tomvothecoder added a commit that referenced this pull request Oct 1, 2024
@tomvothecoder
CDAT Migration Phase 2: Refactor streamflow set (#837)
b89cf3b
tomvothecoder added a commit that referenced this pull request Oct 25, 2024
@tomvothecoder
CDAT Migration Phase 2: Refactor streamflow set (#837)
a00eca8
tomvothecoder added a commit that referenced this pull request Oct 29, 2024
@tomvothecoder
CDAT Migration Phase 2: Refactor streamflow set (#837)
f00dd04
tomvothecoder added a commit that referenced this pull request Oct 29, 2024
@tomvothecoder
CDAT Migration Phase 2: Refactor streamflow set (#837)
0da3a27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tomvothecoder tomvothecoder

Labels
cdat-migration-fy24 CDAT Migration FY24 Task
Projects
None yet
Milestone
FY24 Q4 (07/01/24 - 9/30/24)
Development

Successfully merging this pull request may close these issues.
2 participants
@tomvothecoder @chengzhuzhang