Skip to content

feature_69_lulc_use_case #81

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

Merged
merged 22 commits into from
Aug 7, 2025
Merged

feature_69_lulc_use_case #81

merged 22 commits into from
Aug 7, 2025

Conversation

briannen
Copy link
Collaborator

@briannen briannen commented Jul 18, 2025

Pull Request Testing

This pull request adds the METplus and METplotpy config files for the LULC use case.

  • Describe testing already performed for these changes:

I've thoroughly tested on casper, and everything works as expected.

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

Run through the use case using the new instructions in the documentation.

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

  • Do these changes include sufficient testing updates? [Yes]

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

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

  • Please complete this pull request review by Monday, July 28, 2025.

Pull Request Checklist

  • 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)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Development issue with the original issue number.
  • 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.

@briannen briannen added this to the I-WRF 0.3 milestone Jul 18, 2025
@briannen briannen added component: METplus METplus issues deliverable 2.1 Use Case: Land Use Land Cover (LULC) labels Jul 18, 2025
@briannen briannen linked an issue Jul 18, 2025 that may be closed by this pull request
20 tasks
@briannen
Copy link
Collaborator Author

briannen commented Aug 1, 2025

@georgemccabe I made the updates you suggested!

@georgemccabe georgemccabe requested review from JohnHalleyGotway and removed request for JohnHalleyGotway and jaredalee August 1, 2025 17:07
Copy link
Contributor

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

@briannen I proposed a handful of minor changes to the docs. However, I'm stuck. The commands are not working well on casper or derecho.

In particular, pulling the input data images (lulc-input-obs-d03.apptainer and lulc-input-wrf-d03.apptainer) fail:

> apptainer pull ${IWRF_WORK_DIR}/data-lulc-input-obs.sif docker://ncar/iwrf-data:lulc-input-obs-d03.apptainer
INFO:    Converting OCI blobs to SIF format
INFO:    Starting build...
Copying blob 3698ce01f27f skipped: already exists  
Copying config 44136fa355 done   | 
Writing manifest to image destination
FATAL:   While making image from oci registry: error fetching image to cache: while building SIF from layers: conveyor failed to get: while getting config: unsupported image-specific operation on artifact with type "application/vnd.sylabs.sif.config.v1+json"

I'll note that when I switch from lulc-input-obs-d03.apptainer to lulc-input-obs-d03.docker, then the pull commands DO WORK.

However, then there appear to be problem with the APPTAINER_BIND settings. When I run apptainer exec ... with that environment variable defined as instructed, I always get this error:

FATAL:   While checking container encryption: could not open image /glade/derecho/scratch/johnhg/iwrf_work/data-lulc-input-obs.sif: failed to retrieve path for /glade/derecho/scratch/johnhg/iwrf_work/data-lulc-input-obs.sif: lstat /glade/derecho/scratch/johnhg/iwrf_work/data-lulc-input-obs.sif: no such file or directory

I don't know if that env var setting is related to pulling .docker vs .apptainer images?

Rather than spinning my wheels and guessing, I'm "requesting changes" for this PR. Please re-test to confirm that the commands actually do work on derecho or casper as written.

@JohnHalleyGotway JohnHalleyGotway self-requested a review August 4, 2025 19:28
briannen and others added 2 commits August 4, 2025 14:32

ls ${LOCAL_OUTPUT_DIR}/met_plot/*/*.png -1

This should display a list of PNG image files containing plots and graphics that visualize the verification results. These plots provide graphical representations of the statistical comparisons between the I-WRF LULC simulations and observational data.
Copy link
Contributor

@JohnHalleyGotway JohnHalleyGotway Aug 4, 2025

Choose a reason for hiding this comment

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

The PNG images generated don't have legends to indicate what the different lines mean. I'd recommend adding legends.
Image

Copy link
Contributor

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

@briannen I can confirm that after recent updates, this use case runs on casper.

I did make some more recommendations to:

  • provide users with a rough time estimate to manage their expectations
  • add legends to the plots. It looks like the .yaml files attempt to add the, but I don't see them in the result.
  • either remove or refine the last section on Visualization.

But those recommended changes aside, the basic commands do work.

Thanks for your work on this issue.

Switching the -1 option from the end of the command to the beginning

Co-authored-by: John Halley Gotway <[email protected]>
@briannen
Copy link
Collaborator Author

briannen commented Aug 6, 2025

@JohnHalleyGotway I'm having trouble figuring out why the legend isn't showing up on the plots. @georgemccabe Any ideas?

@georgemccabe
Copy link
Contributor

@JohnHalleyGotway I'm having trouble figuring out why the legend isn't showing up on the plots. @georgemccabe Any ideas?

@briannen, I-WRF is currently using METplus 6.0 which includes METplotpy 3.0. It looks like there was a bug where 0/1 values needed to be used for show_legend instead of True/False. See this discussion comment. It looks like it was fixed in this issue for METplus 6.1.

Can you try replacing the values for show_legend to 1 instead of 'True' and see if that displays the legends properly?

Another option would be to upgrade to METplus 6.1 to get the fix, but I hesitate to do that right now without proper testing. We should make sure to change the values back to True/False if/when we upgrade to using METplus 6.1 for I-WRF.

@JohnHalleyGotway
Copy link
Contributor

@briannen, FYI, me merging PR #83 has introduced some expected differences and conflicts. I'm working on resolving those now and will push a fix soon.

@briannen
Copy link
Collaborator Author

briannen commented Aug 7, 2025

Thanks @georgemccabe, that did the trick! I think it should be ready to go now!

@briannen
Copy link
Collaborator Author

briannen commented Aug 7, 2025

@JohnHalleyGotway Updated!

Copy link
Contributor

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

Thanks @georgemccabe for the tip and thanks @briannen for the update. I can confirm that the legends are now present. I approve.

Image

@briannen briannen merged commit 3ecf3b3 into main Aug 7, 2025
3 checks passed
@briannen briannen deleted the feature_69_lulc_use_case branch August 7, 2025 19:25
@briannen
Copy link
Collaborator Author

briannen commented Aug 7, 2025

Thanks for your help @JohnHalleyGotway and @georgemccabe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: METplus METplus issues deliverable 2.1 Use Case: Land Use Land Cover (LULC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LULC: Define Verification Tasks
3 participants