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

Fix unit conversion logic #579

Conversation

MarcelCaron-NOAA
Copy link
Contributor

@MarcelCaron-NOAA MarcelCaron-NOAA commented Oct 3, 2024

Note to developers: You must use this PR template!

Description of Changes

Please include a summary of the changes and the related GitHub issue(s). Please also include relevant motivation and context.

This fix matches the fix made in PR #521. It addresses a logic error affecting plotting output for five EVS components. The details are reprinted here.

The logic error was found in several instances of code used for converting the units of certain METplus summary statistics. Values of these stats were already converted before being used in follow-up conversion equations (incorrect). As a result, some output plots were displaying incorrect "doubly converted" values of verification metrics.

In this fix, METplus summary statistics are first stored as new variables prior to conversion, and these new variables are used when performing unit conversion and are never overwritten. As a result, all EVS plots affected by the issue should display correct converted values of verification metrics.

Developer Questions and Checklist

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

Yes. Currently, some EVS plots are inaccurate if they display SL1L2- or VL1L2-based metrics and apply unit conversion. The issue affects five EVS components.

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

No. My work schedule may be impacted by the Hurricane Helene recovery timeline (will edit when this is no longer the case).

  • Are there any changes needed for when the jobs are supposed to run?

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 approriate 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

Please include testing instructions for the PR assignee. Include all relevant input datasets needed to run the tests.

(1) Set up jobs

  • symlink the EVS_fix directory locally as "fix"
  • In each driver script, edit the following environment variables:
    HOMEevs - set to your test EVS directory
    COMIN - set to /lfs/h1/ops/prod/com/$NET/$evs_ver_2d
    COMOUT - set to your test output directory
    SENDMAIL - (optional) set to "NO"

(2) Running jobs

  • I recommend testing the following jobs:
jevs_cam_grid2obs_plots
jevs_mesoscale_grid2obs_plots
jevs_global_ens_atmos_gefs_grid2obs_past31days_plots
jevs_analyses_grid2obs_plots
jevs_aqm_grid2obs_plots

[Total: 5 plots jobs]

  • All driver scripts can be submitted using qsub -v vhr=00 $driver
  • All jobs can be submitted at any time

(3) Checking jobs

Log files should be checked by the developer for the following keywords:

check="FATAL\|WARNING\|error\|Killed\|Cgroup\|argument expected\|No such file\|cannot\|failed\|unexpected\|exceeded"
grep "$check" $logfile

Additionally, a sample of test output plots should be compared to EVS prod output.

Note: The changes in this PR affect unit conversion, so plots with no unit conversion will look the same. Whether or not a plot is affected also depends on the metric shown; metrics computed with ffbar, oobar, or fobar are affected (e.g., rmse), but metrics computed with only fbar and obar are not (e.g., bias).

@malloryprow malloryprow self-assigned this Oct 3, 2024
@malloryprow malloryprow added the bug Something isn't working label Oct 3, 2024
@malloryprow malloryprow added this to the EVS v1.0.unit_fix milestone Oct 3, 2024
Copy link

@AndrewBenjamin-NOAA AndrewBenjamin-NOAA left a comment

Choose a reason for hiding this comment

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

These changes match #521 . I approve these changes with successful testing of the impacted plotting components.

@malloryprow
Copy link
Contributor

jevs_cam_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/cam/jevs_cam_grid2obs_plots.o157072007
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0/plots/cam
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_cam_grid2obs_plots.157072007.cbqs01

@malloryprow
Copy link
Contributor

jevs_mesoscale_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/mesoscale/jevs_mesoscale_grid2obs_plots.o157073250
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0/plots/mesoscale
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_mesoscale_grid2obs_plots.157073250.cbqs01

Tagging @PerryShafran-NOAA for awareness

@malloryprow
Copy link
Contributor

jevs_global_ens_atmos_gefs_grid2obs_past31days_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/global_ens/jevs_global_ens_atmos_gefs_grid2obs_past31days_plots.o157073480
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_global_ens_atmos_gefs_grid2obs_past31days_plots.157073480.cbqs01

Tagging @GwenChen-NOAA for awareness

@malloryprow
Copy link
Contributor

malloryprow commented Oct 3, 2024

jevs_analyses_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/analyses/jevs_analyses_grid2obs_plots.o157073976
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_analyses_grid2obs_plots.157073976.cbqs01

Tagging @PerryShafran-NOAA for awareness

@malloryprow
Copy link
Contributor

jevs_aqm_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/aqm/jevs_aqm_grid2obs_plots.o157074109
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_aqm_grid2obs_plots.157074109.cbqs01

Tagging @Ho-ChunHuang-NOAA for awareness

@MarcelCaron-NOAA
Copy link
Contributor Author

The following completed cleanly and output looks good:
✔️ jevs_global_ens_atmos_gefs_grid2obs_past31days_plots

The following failed but seems to just need COMIN corrected:
⚠️ jevs_analyses_grid2obs_plots

export COMIN=/lfs/h1/ops/prod/com/${NET}/${evs_ver_2d} (remove "$USER")

Still running:
⏳ jevs_cam_grid2obs_plots
⏳ jevs_mesoscale_grid2obs_plots
⏳ jevs_aqm_grid2obs_plots

@malloryprow
Copy link
Contributor

jevs_analyses_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/analyses/jevs_analyses_grid2obs_plots.o157073976 COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0 DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_analyses_grid2obs_plots.157073976.cbqs01

Tagging @PerryShafran-NOAA for awareness

Rerun with correct COMIN

jevs_analyses_grid2obs_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/analyses/jevs_analyses_grid2obs_plots.o157075692
COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0
DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_analyses_grid2obs_plots.157075692.cbqs01

Tagging @PerryShafran-NOAA for awareness

@GwenChen-NOAA
Copy link
Contributor

jevs_global_ens_atmos_gefs_grid2obs_past31days_plots

Log File: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/EVS/dev/drivers/scripts/plots/global_ens/jevs_global_ens_atmos_gefs_grid2obs_past31days_plots.o157073480 COMOUT: /lfs/h2/emc/vpppg/noscrub/mallory.row/verification/EVS_PRs/pr579/evs/v1.0 DATA: /lfs/h2/emc/stmp/mallory.row/evs_test/prod/tmp/jevs_global_ens_atmos_gefs_grid2obs_past31days_plots.157073480.cbqs01

Tagging @GwenChen-NOAA for awareness

Plots look good to me.

@Ho-ChunHuang-NOAA
Copy link
Contributor

The AQM plots looks good to me

https://www.emc.ncep.noaa.gov/users/verification/air_quality/aqm/pr/grid2obs/pm25/

@MarcelCaron-NOAA
Copy link
Contributor Author

MarcelCaron-NOAA commented Oct 3, 2024

Thanks @Ho-ChunHuang-NOAA!
✔️ jevs_aqm_grid2obs_plots


⏳ jevs_cam_grid2obs_plots
⏳ jevs_mesoscale_grid2obs_plots
⏳ jevs_analyses_grid2obs_plots

@AliciaBentley-NOAA
Copy link
Contributor

@MarcelCaron-NOAA @Ho-ChunHuang-NOAA Thanks! Can you please remind us how this particular PR should impact AQM? I was thinking that this PR primarily fixed temperature and dewpoint units on plots. Is there an AQM plot that should have changed?

@malloryprow @AndrewBenjamin-NOAA

@MarcelCaron-NOAA
Copy link
Contributor Author

@MarcelCaron-NOAA @Ho-ChunHuang-NOAA Thanks! Can you please remind us how this particular PR should impact AQM? I was thinking that this PR primarily fixed temperature and dewpoint units on plots. Is there an AQM plot that should have changed?

@malloryprow @AndrewBenjamin-NOAA

@AliciaBentley-NOAA That's right, no aqm plot should change. The changes in this PR affect unit conversion, so plots with no unit conversion will look the same. It also depends on the metric shown; metrics computed with ffbar, oobar, or fobar are affected (e.g., rmse), but metrics computed with only fbar and obar are not (e.g., bias).

Even though unit conversion is not currently used in aqm plots, the same faulty unit conversion logic exists and is corrected here.

@malloryprow
Copy link
Contributor

It should be the exact same thing as ops, but a script was edited so good to test to make sure it didn't introduced anything unwanted.

@AliciaBentley-NOAA
Copy link
Contributor

That makes sense. I found that the test plots and the prod plots for AQM looked exactly the same and was I worried that I was missing something (since the plots have been changing in most of the other unit PRs). Good to know that this component needed to look the same to be correct. Thank you!

@MarcelCaron-NOAA
Copy link
Contributor Author

MarcelCaron-NOAA commented Oct 3, 2024

Yes! Those plots should not change. I updated the PR instructions to include that info. Thanks!

@MarcelCaron-NOAA
Copy link
Contributor Author

MarcelCaron-NOAA commented Oct 3, 2024

The following completed cleanly and output look normal:
✔️ jevs_analyses_grid2obs_plots


⏳ jevs_cam_grid2obs_plots
⏳ jevs_mesoscale_grid2obs_plots

@MarcelCaron-NOAA
Copy link
Contributor Author

The following completed cleanly and output look normal:
✔️ jevs_mesoscale_grid2obs_plots


⏳ jevs_cam_grid2obs_plots

@Ho-ChunHuang-NOAA
Copy link
Contributor

@AliciaBentley-NOAA @MarcelCaron-NOAA AQM plots use Marcel's code as the core modules. AQM uses only fbar and obar. But it would be good to sync the updated code with Marcel 's, and thank you Marcel for doing the lift.

@MarcelCaron-NOAA
Copy link
Contributor Author

@malloryprow The following completed cleanly and output look normal:
✔️ jevs_cam_grid2obs_plots

👍 All of the test jobs have completed successfully

Copy link
Contributor

@malloryprow malloryprow left a comment

Choose a reason for hiding this comment

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

Changes are good and testing successful.

@malloryprow malloryprow merged commit d1ec41f into NOAA-EMC:release/evs.1.0.unit_fix Oct 4, 2024
@malloryprow
Copy link
Contributor

Thanks for your work on this @MarcelCaron-NOAA!

@MarcelCaron-NOAA MarcelCaron-NOAA deleted the release/evs.1.0.cam_unit_fixes branch December 2, 2024 21:38
@malloryprow malloryprow modified the milestones: EVS v1.0.15, EVS v1.0.16 Dec 9, 2024
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.

analyses, aqm, cam, global_ens, mesoscale: Fix plots unit conversion for operations
6 participants