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: Fix African easterly wave density plots in TC analysis and convert H20LNZ units to ppm/volume #882

Merged
merged 3 commits into from
Oct 29, 2024
Merged

CDAT migration: Fix African easterly wave density plots in TC analysis and convert H20LNZ units to ppm/volume #882

merged 3 commits into from
Oct 29, 2024

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Oct 29, 2024 •
edited
Loading

Description

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 changed the title Fix African easterly wave density plots in TC analysis CDAT migration: Fix African easterly wave density plots in TC analysis and Convert H20LNZ units to ppm/volume Oct 29, 2024
@tomvothecoder tomvothecoder changed the title CDAT migration: Fix African easterly wave density plots in TC analysis and Convert H20LNZ units to ppm/volume CDAT migration: Fix African easterly wave density plots in TC analysis and convert H20LNZ units to ppm/volume Oct 29, 2024
tomvothecoder
tomvothecoder commented Oct 29, 2024
View reviewed changes
Copy link
Collaborator Author

@tomvothecoder tomvothecoder left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @chengzhuzhang this PR is ready for review. I've performed a self-review, but good to make sure the changes align with your two PRs mentioned in this PR's description.

Summary of changes

  • tc_analysis_driver.py
    • Remove lat/lon slice
    • Add function _filter_lines_within_year_bounds() to remove excessive time points crossing year bounds for 6 hourly data
  • zonal_mean_2d_driver.py
    • Update _convert_g_kg_to_ppm_units() to convert to ppm by volume.

e3sm_diags/driver/zonal_mean_2d_driver.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes should align with https://github.com/E3SM-Project/e3sm_diags/pull/874/files#diff-794c0344b0a718e616d2051d6961d02c79f24e8d5e97c7c355fa7cb0ec467e19

e3sm_diags/driver/tc_analysis_driver.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes should align with https://github.com/E3SM-Project/e3sm_diags/pull/851/files

Note, #872 was already addressed on this branch.

@tomvothecoder tomvothecoder marked this pull request as ready for review October 29, 2024 19:02
@tomvothecoder tomvothecoder added the cdat-migration-fy24 CDAT Migration FY24 Task label Oct 29, 2024
@tomvothecoder tomvothecoder self-assigned this Oct 29, 2024
tomvothecoder
tomvothecoder commented Oct 29, 2024
View reviewed changes
e3sm_diags/driver/tc_analysis_driver.py
Comment on lines +233 to +267
def _filter_lines_within_year_bounds(
lines_orig: List[str], data_end_year: int
) -> List[str]:
"""Filters lines within the specified year bounds.

This function processes a list of strings, each representing a line of data.
It filters out lines based on a year extracted from each line, ensuring that
only lines with years less than or equal to `data_end_year` are retained.
Additionally, it removes excessive time points crossing year bounds from
6-hourly data.

Parameters
----------
lines_orig : List[str]
A list of strings where each string represents a line of data.
data_end_year : int
The end year for filtering lines. Only lines with years less than or
equal to this value will be retained.
Returns
-------
List[str]
A list of strings filtered based on the specified year bounds.
"""
line_ind = []
for i in range(0, np.size(lines_orig)):
if lines_orig[i][0] == "s":
year = int(lines_orig[i].split("\t")[2])

if year <= data_end_year:
line_ind.append(i)

end_ind = line_ind[-1]

new_lines = lines_orig[0:end_ind]
return new_lines
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI I used GitHub Co-Pilot to extract this function and generated the docstring. I've found it to be useful for these kinds of tasks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice finding!

@tomvothecoder tomvothecoder mentioned this pull request Oct 29, 2024
Closed
chengzhuzhang
chengzhuzhang requested changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@chengzhuzhang chengzhuzhang left a comment

Choose a reason for hiding this comment

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

@tomvothecoder I think the changes in .cfg files for H2OLNZ units change are not brought in this PR: https://github.com/E3SM-Project/e3sm_diags/pull/874/files#diff-794c0344b0a718e616d2051d6961d02c79f24e8d5e97c7c355fa7cb0ec467e19

@tomvothecoder
Copy link
Collaborator Author

@tomvothecoder I think the changes in .cfg files for H2OLNZ units change are not brought in this PR: #874 (files)

For that specific PR, the changes are already here. Example:

e3sm_diags/e3sm_diags/driver/default_diags/zonal_mean_2d_stratosphere_model_vs_obs.cfg

Lines 65 to 75 in 3df16d2

[#]
sets = ["zonal_mean_2d_stratosphere"]
case_id = "MERRA2 (relative difference)"
variables = ["H2OLNZ"]
ref_name = "MERRA2"
reference_name = "MERRA2 Reanalysis"
seasons = ["ANN", "01", "02", "03", "04", "05", "06", "07", "08", "09", "10", "11", "12", "DJF", "MAM", "JJA", "SON"]
diff_type = "relative"
diff_title = "(Model - Obs.)/Obs. * 100"
contour_levels = [0.5,1,1.5,2,2.5,3,3.5,4,4.5,5,5.5,6,7,8]
diff_levels = [-100, -75, -50, -30, -15, -5, 5, 15, 30, 50, 75, 100]

I rebased the dev branch on the latest main, which pulls in changes to .cfg files. I don't need to update any .cfg files in this PR.

chengzhuzhang
chengzhuzhang approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@chengzhuzhang chengzhuzhang left a comment

Choose a reason for hiding this comment

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

Looking good! The PR is good to go!

@chengzhuzhang
Copy link
Contributor

@tomvothecoder Just a heads-up that this PR is also just merged: #879 and we need to bring to migration branch..

@tomvothecoder tomvothecoder merged commit d5a1aad into cdat-migration-fy24 Oct 29, 2024
4 checks passed
@tomvothecoder tomvothecoder deleted the feature/851-874 branch October 29, 2024 21:17
tomvothecoder added a commit that referenced this pull request Oct 29, 2024
@tomvothecoder
CDAT migration: Fix African easterly wave density plots in TC analysi…
7550b3d 
…s and convert H20LNZ units to ppm/volume (#882)
@tomvothecoder
Copy link
Collaborator Author

@tomvothecoder Just a heads-up that this PR is also just merged: #879 and we need to bring to migration branch..

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chengzhuzhang chengzhuzhang chengzhuzhang approved these changes

Assignees

@tomvothecoder tomvothecoder

Labels
cdat-migration-fy24 CDAT Migration FY24 Task
Projects
None yet
Milestone
FY25 Q1 (10/1/24 - 12/31/24)
Development

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