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

bugfix/long_term_stats #663

Merged

Conversation

malloryprow
Copy link
Contributor

Note to developers: You must use this PR template!

Description of Changes

This corrects some items for the global_det long-term stats. It uses stat_analysis to aggregate FSS and doesn't require a minimum number of days to calculate for precip. This matches the methods that had been previously done by verf_precip. It also fixes an issue with the 3 inch conversion to mm.

Developer Questions and Checklist

  • Is this a high priority PR? If so, why and is there a date it needs to be merged by?

No

  • Do you have any planned upcoming annual leave/PTO?

No

  • Are there any changes needed in the times when the jobs are supposed to run/kick-off?

No

  • The code changes follow NCO's EE2 Standards.
  • Developer's name is removed throughout the code and have used ${USER} where necessary throughout the code.
  • References the feature branch for HOMEevs are removed from the code.
  • J-Job environment variables, COMIN and COMOUT directories, and output follow what has been defined for EVS.
  • Jobs over 15 minutes in runtime have restart capability.
  • If applicable, changes in the dev/drivers/scripts or dev/modulefiles have been made in the corresponding ecf/scripts and ecf/defs/evs-nco.def?
  • Jobs contain the appropriate file checking and don't run METplus for any missing data.
  • Code is using METplus wrappers structure and not calling MET executables directly.
  • Log is free of any ERRORs or WARNINGs.

Testing Instructions

Set-up

  1. Clone my fork and checkout branch bugfix/long_term_stats
  2. ln -sf /lfs/h2/emc/vpppg/noscrub/emc.vpppg/verification/EVS_fix fix

🔲 global_det stats

  1. cd dev/drivers/scripts/stats/global_det
  2. Run jevs_global_det_atmos_long_term_stats.sh (takes a few hours to run)
  • Set COMIN to /lfs/h2/emc/vpppg/noscrub/emc.vpppg/verification/global/archive/evs_data

@malloryprow malloryprow added the bug Something isn't working label Feb 10, 2025
@malloryprow malloryprow added this to the EVS v2.0.0 milestone Feb 10, 2025
@malloryprow
Copy link
Contributor Author

CC: @QiShi-NOAA for you awareness

@PerryShafran-NOAA
Copy link
Contributor

The job is underway.

Copy link
Contributor

@AliciaBentley-NOAA AliciaBentley-NOAA left a comment

Choose a reason for hiding this comment

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

I have reviewed the changes made in this PR and found them to be consistent with the stated fixes in the submission! The associated long-term QPF plots look much better than they did on the Headline Score webpage. If the results of Perry's test are good, then I approve this PR to be merged. Thanks!

CC @PerryShafran-NOAA @malloryprow

Copy link
Contributor

@AliciaBentley-NOAA AliciaBentley-NOAA left a comment

Choose a reason for hiding this comment

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

Haha... how about I approve it this time instead of just comment? :)

@PerryShafran-NOAA
Copy link
Contributor

@malloryprow Based on Alicia's comment, should I also run the plots as part of this test?

@malloryprow
Copy link
Contributor Author

I don't think so, the problem was in the stats calculation not how the plots was displaying them.

@PerryShafran-NOAA
Copy link
Contributor

OK great - I was just wondering if seeing the plots display the stats was the best way to see that the stats corrections were OK.

@malloryprow
Copy link
Contributor Author

Nah, I can look in the text files.

@PerryShafran-NOAA
Copy link
Contributor

The job is still running, but here is the information as to where to find all the output.

.o file:
/lfs/h2/emc/vpppg/noscrub/perry.shafran/pr663test/EVS/dev/drivers/scripts/stats/global_det/jevs_global_det_atmos_long_term_stats_00.o6893876
output: /lfs/h2/emc/vpppg/noscrub/perry.shafran/evs/v2.0/stats/global_det/long_term
working directory: /lfs/h2/emc/stmp/perry.shafran/evs_test/dev/tmp/jevs_global_det_atmos_long_term_stats_00.6893876.dbqs01

@PerryShafran-NOAA
Copy link
Contributor

@malloryprow The job has been completed.

@malloryprow
Copy link
Contributor Author

Log and stat files look good to me!

@PerryShafran-NOAA
Copy link
Contributor

All right! I will review and then merge if all is good.

Copy link
Contributor

@PerryShafran-NOAA PerryShafran-NOAA left a comment

Choose a reason for hiding this comment

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

Changes work as expected. Approved for merge.

@PerryShafran-NOAA PerryShafran-NOAA merged commit 2242d3e into NOAA-EMC:develop Feb 10, 2025
@malloryprow malloryprow deleted the bugfix/long_term_stats branch February 10, 2025 18:48
BinbinZhou-NOAA added a commit to BinbinZhou-NOAA/EVS that referenced this pull request Feb 20, 2025
…AA/EVS into feature/narre_MPMD

* 'feature/narre_MPMD' of https://github.com/BinbinZhou-NOAA/EVS:
  Update naming conventions per Executive Order 14172 (NOAA-EMC#666)
  GEFS -CHEM PLOTS MPMD BUGFIX - Feature/gef splotmpmd (NOAA-EMC#664)
  bugfix/long_term_stats (NOAA-EMC#663)
  Global-det SST sea ice fix (NOAA-EMC#660)
  bugfix/dev_fix_ghrsst (NOAA-EMC#662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants