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

Met plus tc #31

Closed
wants to merge 11 commits into from
Closed

Met plus tc #31

wants to merge 11 commits into from

Conversation

JiayiPeng-NOAA
Copy link
Contributor

Pull Request Testing

  • Describe testing already performed for this Pull Request:

    Yes. The 3 jobs have been tested.

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

  • Has the code been checked to ensure that no errors occur during the execution? [Yes or No]
    Yes.

  • Do these updates/additions include sufficient testing updates? [Yes or No]
    No.

  • Please complete this pull request review by [Dec. 01, 2022].

Pull Request Checklist

  • Review the source issue metadata (required labels, projects, and milestone).

  • Complete the PR description above.

  • Ensure the PR title matches the feature branch name.

  • Check the following:

  • Instructions provided on how to run

  • Developer's name is replaced by ${user} where necessary throughout the code

  • Check that the ecf file has all the proper definitions of variables

  • Check that the jobs file has all the proper settings of COMIN and COMOUT and other input variables

  • Check to see that the output directory structure is followed

  • Be sure that you are not using MET utilities outside the METplus wrapper structure

  • 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.

@PerryShafran-NOAA
Copy link
Contributor

Hi, Jiayi,

There are a few issues that need to be resolved before I work on this pull request.

First - you have your username in the J-job file. We understand that these data files likely reside in your directory, but you would need to set this in the ecf file, and have non-user directories in the J-job. Such as:

export COMINgenesis=${COMINgenesis:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/genesisDATA}
export COMINadeckNHC=${COMINadeckNHC:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/ABdeck}
export COMINbdeckNHC=${COMINbdeckNHC:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/ABdeck}
export COMINadeckJTWC=${COMINadeckJTWC:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/ABdeck}
export COMINbdeckJTWC=${COMINbdeckJTWC:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/ABdeck}

Second - there are references to specific version numbers in your code. You need to bring that in from the run.ver file and use that variable instead of the hard-wired version number. Such as:

export COMINvit=${COMINvit:-/lfs/h1/ops/prod/com/gfs/v16.2/syndat/syndat_tcvitals.${YYYY}}
export COMINtrack=${COMINtrack:-/lfs/h1/ops/prod/com/ens_tracker/v1.3/global/tracks.atcfunix.${YY22}

Would you please make these changes and commit them to your feature branch?

Thanks so much! Let me know if you have any questions. Respond here so we have the written record.

Perry

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 4, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

In order for PRs to be tested, these parts of the code need to be in compliance with EVS coding regulations. Please make the changes in order for any test to be performed.

Perry

@ShelleyMelchior-NOAA
Copy link
Contributor

This construct must be corrected and will not be accepted by EIB or NCO, and for consistency across all aspects of EVS, should even be adhered to for non-operational code as much as is reasonable.
export COMINgenesis=${COMINgenesis:-/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/genesisDATA}
This says: set COMINgenesis to a value set upstream, and if not set upstream, then set it to the default definition.
In pseudo code:
export COMINgenesis=${COMINgenesissetupstream:-/default/path/for/genesisDATA}
How you have COMINgenesis defined is such that your user directory, /lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/genesisDATA, is the default location. NCO will not support that as the default.
I understand this is for code that will not be delivered to NCO, so we can relax this a little bit. But as Perry said, we should conform to standards as much as we can. I suggest this change:
export COMINgenesis=${COMINgenesis:-/lfs/h2/emc/vpppg/save/$USER/TCgen/genesisDATA}
If a different EVS developer runs your code, all they have to do is export COMINgenesis from their ecf script, as you correctly noted. In the ecf script they can set COMINgenesis to /lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/genesisDATA, getting your name out of the J-job script.

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 8, 2022 via email

@ShelleyMelchior-NOAA
Copy link
Contributor

I appreciate your attention to this, Jaiyi! Now I want to apologize. I wasn't clear enough. When I wrote out the pseudo-code, it was just to help convey what the construct means. It wasn't how the J-job scripts should have been changed. So let me be extra clear, hopefully. In you J-job scripts ...
This
export COMINgenesis=${COMINgenesis:-/your/TC/genesis/data/dir}
should look like this
export COMINgenesis=${COMINgenesis:-/lfs/h2/emc/vpppg/save/$USER/TCgen/genesisDATA}

I imagine most users will not have data in /lfs/h2/emc/vpppg/save/$USER/TCgen/genesisDATA like you do, so a different user will need to export COMINgenesis from their ecf to be defined as
export COMINgenesis=/lfs/h2/emc/vpppg/save/jiayi.peng/TCgen/genesisDATA

I hope that is clearer.

@PerryShafran-NOAA
Copy link
Contributor

Hi, Jiayi,

Another change we are asking you to make is that we need to reduce the number of J-jobs in each system. Therefore, you will need to combine your GFS and GEFS jobs into one J-job. You should use the ecf script to define whether it is for GFS or GEFS, and also define the COMIN/COMOUT directories there. Then we'll have fewer J-jobs to worry about. You should be able to use if statements in the J-job if there is different treatment of GFS or GEFS.

Please try to do this and then re-submit your job. Let me know if you have any questions regarding this request.

Perry

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 9, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

Even though the hurricane verification is not being submitted to NCO, the EVS leadership would prefer to see the GFS and GEFS (and for that matter regional) into one J-job. The work to do this now will reduce the amount of work in the future since there will be fewer J-jobs to keep track of.

If you wish to meet, that is fine, we can do so, but Jason and Alicia clearly would like to see the GFS and GEFS combined into a single J-job. It can also be discussed whether we can put the track and genesis J-jobs into one also.

In any case, I won't be able to review your code until this is resolved.

Thanks!

Perry

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 10, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 15, 2022 via email

@ShelleyMelchior-NOAA
Copy link
Contributor

Hi @JiayiPeng-NOAA -- I was looking through your updated PR to help give Olivia a preview in how things will be modified for regional TC verification. I found a few more things in your PR that would be good to address. I will put the comments in the impacted code files.

ShelleyMelchior-NOAA
ShelleyMelchior-NOAA reviewed Nov 16, 2022
View reviewed changes
jobs/hurricane/stats/JEVS_HURRICANE_STATS
####################################
# Specify Execution Areas
####################################
export HOMEevs=${HOMEevs:-${PACKAGEHOME}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is $PACKAGEHOME defined? This will break if $HOMEevs is not defined in the ecf script.

ShelleyMelchior-NOAA
ShelleyMelchior-NOAA reviewed Nov 16, 2022
View reviewed changes
jobs/hurricane/stats/JEVS_HURRICANE_STATS
export COMINbdeckJTWC=${COMINbdeckJTWC:-/your/JTWC/bdeck/data/dir}

fi

Copy link
Contributor

Choose a reason for hiding this comment

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

This entire if-elif-fi block is good! But I think we need one final catch to allow the job to exit gracefully should none of the cases be satisfied. This is to catch the off chance for a user typo, for example, in the ecf script when defining $COMPONENT and $RUN.

ShelleyMelchior-NOAA
ShelleyMelchior-NOAA reviewed Nov 16, 2022
View reviewed changes
jobs/hurricane/stats/JEVS_HURRICANE_STATS
####################################
export NET=${NET:-evs}
export COMPONENT=${COMPONENT:-define_in_ecf}
export RUN=${RUN:-define_in_ecf}
Copy link
Contributor

@ShelleyMelchior-NOAA ShelleyMelchior-NOAA Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

the default of 'define_in_ecf' will cause this J-job to break b/c a test for 'define_in_ecf' is not in place in the if-elif-fi below. I suggest that you hard code a default for COMPONENT and RUN that will not cause the J-job to break. For example, 'hurricane_global_det' for COMPONENT and 'tropcyc' for RUN.
Alternatively, you can run a quick test right here to make sure that COMPONENT and RUN are previously set to acceptable values, and if not, then force a graceful exit. A check like this is important if certain variables are required to be defined.

ShelleyMelchior-NOAA
ShelleyMelchior-NOAA reviewed Nov 16, 2022
View reviewed changes
jobs/hurricane/stats/JEVS_HURRICANE_STATS
export COMINvit=${COMINvit:-/your/TC/vitals/file}
export COMINtrack=${COMINtrack:-your/TC/track/file}
export COMINbdeckNHC=${COMINbdeckNHC:-/your/NHC/bdeck/data/dir}
export COMINbdeckJTWC=${COMINbdeckJTWC:-/your/JTWC/bdeck/data/dir}
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of default path, e.g., '/your/TC/vintals/file', or '/your/TC/track/file', or '/your/NHC/bdeck/data/dir', or '/your/JTWC/bdeck/data/dir', will cause the downstream processing to have problems if these COMIN* variables are not defined in the ecf script.
We either need to use a default path that indeed exists on the computing system that the verification package is running on, or we need to require that these COMIN* variables be defined in order to proceed. If not defined, gracefully exit.

@PerryShafran-NOAA
Copy link
Contributor

Hi, Jiayi,

I ran all three scripts, and all three seemed to work. Please validate the data (stats and plots) in the following directories, to see that your output is the same as what I have here.

/lfs/h2/emc/ptmp/perry.shafran/jevs_hurricane_global_ens_tropcyc_stats_00.o93132
/lfs/h2/emc/ptmp/perry.shafran/jevs_hurricane_global_det_tropcyc_stats_00.o253230
/lfs/h2/emc/ptmp/perry.shafran/jevs_hurricane_global_det_tcgen_stats_00.o198368

Thanks!

Perry

@ShelleyMelchior-NOAA
Copy link
Contributor

I also managed to run/test this PR today. Provided are the files I generated.

Output log files:
/lfs/h2/emc/obsproc/noscrub/shelley.melchior/githubwkspc/forks/JP/EVS/jevs_hurricane_global_det_tcgen_stats.o19403160
/lfs/h2/emc/obsproc/noscrub/shelley.melchior/githubwkspc/forks/JP/EVS/jevs_hurricane_global_det_tropcyc_stats.o19406034
/lfs/h2/emc/obsproc/noscrub/shelley.melchior/githubwkspc/forks/JP/EVS/jevs_hurricane_global_ens_tropcyc_stats.o19407096

COMOUT=/lfs/h2/emc/ptmp/shelley.melchior/com/evs/1.0.0

@ShelleyMelchior-NOAA
Copy link
Contributor

I think there is still some consternation regarding where the png plot files should reside in $COMOUT. Following my tests I see png plot files in
$COMOUT/hurricane_global_*/t*/stats/*/plot/*png
Is it possible to have the png plot files reside in a directory at the same level as stats? Like this?
$COMOUT/hurricane_global_*/t*/plots/*/*png

where COMOUT=/lfs/h2/emc/ptmp/shelley.melchior/com/evs/1.0.0

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 22, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 22, 2022 via email

@malloryprow
Copy link
Contributor

malloryprow commented Nov 23, 2022 •
edited
Loading

Jiayi's error brings up a good point for any of us making spatial maps of any sort using Cartopy. Does NCO keep the shapefiles Cartopy uses anywhere? If Cartopy doesn't find the files in a specific directory, it tries to download the files, like you see if the error from jevs_hurricane_global_det_tcgen_stats.ecf.

This might clarify it a bit more: SciTools/cartopy#1325. If you specify "from cartopy import config//config['data_dir']" it will look for the data there. If NCO has the data downloaded, we can set our scripts that use Cartopy there. Otherwise if it isn't the default (~/.local/share/cartopy), Cartopy will try to download the files from online and put them there. I don't think NCO would like that. Additionally, the download will fail, like it did in Jaiyi's job, if the job is running in the dev queue since it has no access to the outside.

@AliciaBentley-NOAA
Copy link
Contributor

AliciaBentley-NOAA commented Nov 28, 2022 via email

@LoganDawson-NOAA
Copy link
Contributor

I would guess that NCO would only keep those cartopy files in a common space if there's some production plotting job that's already leveraging cartopy (I'm guessing there's not). It might be a good thing for NCO to manage if they're willing. If not, we can just bundle those shapefiles as part of the EVS_fix delivery

@malloryprow
Copy link
Contributor

@AliciaBentley-NOAA I think I copied the files over from WCOSS to WCOSS2 when I was transitioning my files, so I never had to download them since the files already existed in my ~/.local/share/cartopy/.

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Nov 28, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

OK, so we are now ready to test? The shapefiles are currently in some location and the code now points to them?

Thanks!

Perry

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 8, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

OK, great, I will test.

Perry

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 8, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

Oh, I didn't know there was a script for regional verification. I'll check that one too.

Thanks!

Perry

@PerryShafran-NOAA
Copy link
Contributor

Tests are now underway.

Perry

@PerryShafran-NOAA
Copy link
Contributor

Testing has been completed.

Please review all the stats and plots in the directory /lfs/h2/emc/ptmp/perry.shafran/com/evs/1.0.0 - be sure that everything ran to completion and that stats and plots look identical to your own runs of these scripts.

Thanks!

Perry

@AliciaBentley-NOAA
Copy link
Contributor

AliciaBentley-NOAA commented Dec 8, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 8, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 8, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 8, 2022 via email

@PerryShafran-NOAA
Copy link
Contributor

Yes, instead of the copying, just direct your plots to be made in the plots directory.

@AliciaBentley-NOAA - is Jiayi not making a tar file out of his plots, like the rest of EVS? If that's OK, I was just curious. Should be easy enough to change the directory where the plots go to in that case.

Also a minor change: instead of the 1.0.0 after the evs in the directory structure, that should be "v1.0".

Thanks!

Perry

@AliciaBentley-NOAA
Copy link
Contributor

AliciaBentley-NOAA commented Dec 9, 2022 via email

@JiayiPeng-NOAA
Copy link
Contributor Author

JiayiPeng-NOAA commented Dec 9, 2022 via email

@AliciaBentley-NOAA
Copy link
Contributor

AliciaBentley-NOAA commented Dec 9, 2022 via email

@ShelleyMelchior-NOAA ShelleyMelchior-NOAA mentioned this pull request Dec 13, 2022
Merged
18 tasks
@ShelleyMelchior-NOAA
Copy link
Contributor

I am going to close this PR w/ no merge. This PR has been subsumed by PR #46 .

BinbinZhou-NOAA added a commit to BinbinZhou-NOAA/EVS that referenced this pull request Dec 27, 2023
@BinbinZhou-NOAA
address EVS v1.0 Issues NOAA-EMC#31 and NOAA-EMC#32
621b139
@BinbinZhou-NOAA BinbinZhou-NOAA mentioned this pull request Dec 27, 2023
Merged
18 tasks
ShelleyMelchior-NOAA added a commit that referenced this pull request Dec 28, 2023
@BinbinZhou-NOAA @ShelleyMelchior-NOAA
Feature/ensemble_fix for Wei's document (#33): avoid mkdir if the dir…
99fadfa 
…ectory exists, fix 2 typos in sub-domain names in href plot script files (#398)

* remove met_v in all plotting scripts

* remove met_v from sref's ecf scripts

* remove met_v from sref's ecf scripts

* avoid mkdir for existing directory and 2 typos

* fix one typo in scripts/plots/cam/exevs_href_profile_plots.sh

* Update PointStat_fcstSREF_obsPREPBUFR_cnv.conf to select one from  multiple fields

* address EVS v1.0 Issues #31 and #32

* Updating metar config file to check for 3hr cycle frequency.

---------

Co-authored-by: Shelley Melchior <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShelleyMelchior-NOAA ShelleyMelchior-NOAA ShelleyMelchior-NOAA left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JiayiPeng-NOAA @PerryShafran-NOAA @ShelleyMelchior-NOAA @malloryprow @AliciaBentley-NOAA @LoganDawson-NOAA