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

Feature #2966 local_solar_time #3008

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Feature #2966 local_solar_time #3008

wants to merge 14 commits into from
+629 −327

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Nov 5, 2024 •
edited
Loading

NOTE: DO NOT MERGE THIS PR UNTIL AFTER THE MET-12.0.0-RC1 RELEASE

Add support for new -type solar_time masking type.
Also adds support for multiple -type, -mask_field, and -thresh settings for a single run, when all use the same mask_file setting.

Expected Differences

  • Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]

    If yes, please describe:

  • Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [Yes]

    If yes, please describe:

    Adds new units attribute to the NetCDF output from Gen-Vx-Mask.
    Updates the mask_type attribute with additional details about the data processed.
    Adds logic to concatenate masking type names when constructing the output variable name.

Pull Request Testing

  • Describe testing already performed for these changes:

    Tested manually.

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

  1. Confirm all differences in GitHub actions are expected:
  • 1 new Gen-Vx-Mask output file.
  • A units NetCDF variable attribute is added to existing Gen-Vx-Mask output files.
  1. Carefully review updated Gen-Vx-Mask usage statement and User's Guide documentation for typos, accuracy, and clarity.
  2. Test out seneca:/d1/projects/MET/MET_pull_requests/met-12.1.0/beta1/MET-feature_2966_local_solar_time/bin/gen_vx_mask and try to "break it" ensuring it behaves as expected.

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
    Updated the chapter about spatial masking.

  • Do these changes include sufficient testing updates? [Yes]
    Added two new Gen-Vx-Mask unit test to demonstrate:

  • combining the -type solar_alt and -type solar_time options in a single run

  • combining data/data/lat/lon masking types in a single run

  • Will this PR result in changes to the MET test suite? [Yes]

    If yes, describe the new output and/or changes to the existing output:

Two new files described above and changes to the units and mask_type attributes of existing output files.

  • Will this PR result in changes to existing METplus Use Cases? [Yes or No]

    If yes, create a new Update Truth METplus issue to describe them.
    The units variable attribute will be added and mask_type modified for existing Gen-Vx-Mask output files.

  • Do these changes introduce new SonarQube findings? [No]

    If yes, please describe:
    While existing SonarQube findings will be flagged as being "new", the overall number of code smells is reduced from 18,315 to about 18,251.

  • Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s) and Development issue
    Select: Milestone as the version that will include these changes
    Select: Coordinated METplus-X.Y Support project for bugfix releases or MET-X.Y.Z Development project for official releases
  • After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@JohnHalleyGotway
Per #2966, add new solar_time() function to the vx_solar library.
2c13fda
@JohnHalleyGotway
Per #2966, add support for new solar_time masking type. Also make log…
4b469d6 
… messages for consistent and eliminate the warning about -thresh not being specified becuase its fine to not specify a threshold.
@JohnHalleyGotway
Per #2966, add a units attribute to the output NetCDF mask variable.
2328400
@JohnHalleyGotway
Per #2966, modify solar azimuth and altitude strings to make the log …
ea9840d 
…messages align well.
@JohnHalleyGotway
Per #2966, add gen_vx_mask unit test to demonstrate the solar_time ma…
0ed3fe2 
…sking type.
@JohnHalleyGotway
Per #2966, add documentation about the -solar_time option
2a32e45
@JohnHalleyGotway
Per #2966, reduce SonarQube code smells in gen_vx_mask
8734801
@JohnHalleyGotway
Per #2966, reduce SonarQube findings
e865e98
@JohnHalleyGotway
Per #2966, support multiple mask types with the same mask field being…
a64930f 
… supported in a single run. Still need to update the user's guide.
@JohnHalleyGotway
Per #2966, add UTC
42ff3cf
@JohnHalleyGotway
Per #2966, update gen_vx_mask docs about supporting multiple -type op…
b1268a8 
…tions in a single run
@JohnHalleyGotway JohnHalleyGotway added this to the MET-12.1.0 milestone Nov 5, 2024
@JohnHalleyGotway JohnHalleyGotway linked an issue Nov 5, 2024 that may be closed by this pull request
Enhance gen_vx_mask to support a new local solar time masking region option #2966
Open
22 tasks
@JohnHalleyGotway JohnHalleyGotway mentioned this pull request Nov 6, 2024
Open
22 tasks
@JohnHalleyGotway
Per #2966, update logic to fix using data masking twice, add a unit t…
4ff2c26 
…est to demonstrate, and update the mask_type attribute to include the magic string for the gridded data used for data masking.
@JohnHalleyGotway
Merge branch 'feature_2966_local_solar_time' of https://github.com/dt…
120d132 
…center/MET into feature_2966_local_solar_time
@JohnHalleyGotway JohnHalleyGotway requested review from CPKalb and removed request for jvigh November 6, 2024 19:45
@JohnHalleyGotway
Copy link
Collaborator Author

Based on this issue comment, requesting this PR review from @CPKalb. Tina, please LMK if you have any questions or run into any issues.

@JohnHalleyGotway
Copy link
Collaborator Author

JohnHalleyGotway commented Nov 6, 2024 •
edited
Loading

I'd previously found some differences in the presence of timing attributes being added to the Gen-Vx-Mask output files. I updated the logic with this commit to eliminate those diffs.

The remaining diffs flagged by the GHA testing workflow are all expected, including the addition of the units NetCDF variable attribute and changes to the mask_type attribute for data masking, like this:

COMPARING gen_vx_mask/DATA_INPUT_FIELD_APCP_24_where_TMP_Z2_le300.nc
file1: /data/output/met_test_truth/gen_vx_mask/DATA_INPUT_FIELD_APCP_24_where_TMP_Z2_le300.nc
file2: /data/output/met_test_output/gen_vx_mask/DATA_INPUT_FIELD_APCP_24_where_TMP_Z2_le300.nc
ERROR: NetCDF headers differ:
16a17
> 		APCP_24_where_TMP_Z2_le300:units = "kg/m^2" ;
18c19
< 		APCP_24_where_TMP_Z2_le300:mask_type = "data>300" ;
---
> 		APCP_24_where_TMP_Z2_le300:mask_type = "data(TMP/Z2)>300" ;

@JohnHalleyGotway
Per #2966, adjust the logic slightly to revert to existing behavior w…
0177be5 
…here we only write the timing information of the input data to the gen_vx_mask output when no threshold was applied. This should reduce the number of diffs flagged by PR #3008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPKalb CPKalb Awaiting requested review from CPKalb

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
MET-12.1.0 Development
Status: 🔎 In review
Milestone
MET-12.1.0
Development

Successfully merging this pull request may close these issues.

Enhance gen_vx_mask to support a new local solar time masking region option
1 participant
@JohnHalleyGotway