Feature #2147 HSS

[JohnHalleyGotway]

Expected Differences

• Do these changes introduce new tools, command line arguments, or configuration file options? [No]
  
  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 EC_VALUE column to the existing CTC line type (just like it had been added to the MCTC line type previously).
  • Adds new HSS_EC, HSS_EC_BCL, HSS_EC_BCU, and EC_VALUE columns to the existing CTS line type (just like they had been added to the MCTS line type previously).

Pull Request Testing

• Describe testing already performed for these changes:
  

  • Confirmed the inclusion of these new CTC and CTS columns in make test output.
  • Re-ran Point-Stat/Grid-Stat after modifying the default hss_ec_value = NA;. The default of NA results in hss_ec_value = 0.5;. Confirmed that lowering it to 0.20 increases the resulting HSS_EC and increasing it to 0.70 lowers HSS_EC, as you would expect.
  • Manually ran Stat-Analysis jobs.
    • For aggregate (CTC and MCTC) and aggregate_stat (CTC->CTS and MCTC->MCTS) jobs, let the -hss_ec_value job command option override what the input data says. Note that if multiple values appear in the input, the FIRST value encountered will be used for the output.
    • For aggregate_stat (MPR->CTC/CTS and MPR->MCTC/MCTS) jobs, let the -hss_ec_value job commend option override the default value of NA (which results in EC_VALUE = 1/n_bins).

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

  • Does the EC_VALUE column actually belong in the CTC and MCTC line types?
    • Per discussion with @j-opatz, yes it does so that it can be aggregated well by METplus Analysis tools.
  • Is the computed HSS_EC stat actually correct? Is there anyway to independently verify the computation?
  • Is defining hss_ec_value as a single floating point number sufficient? Or should we allow hss_ec_value to be defined as an array with the same length as cat_thresh so that users could easily define the expected correct for each threshold?
    • Per discussion with @j-opatz, yes, the current option of defining it a single floating point number is fine for it's intended usage at CPC.
  • Review and validate the code changes.
  • Review and validate the changes to the existing unit test output.
  • Review the changes to the documentation in this RTD branch.

• 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 or No]

I'm not sure about this. Should I add a test to demonstrate that changes to the hss_ec_value really do change the result? If so, any advice on where?

• Will this PR result in changes to the test suite? [Yes]
  
  If yes, describe the new output and/or changes to the existing output:
  
  Lots and lots of changes... Every CTC and CTS output line will be modified.

• Please complete this pull request review by [Fill in date].
  
  Not sure. I'll be out of the office 5/30-6/3. If we can't do this before, we could do it after.

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)
  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 Linked issues 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.

[JohnHalleyGotway]

@j-opatz I marked this as draft because I'm not sure whether or not you'll want more changes. Need to decided:

1. Should we support hss_ec_value being defined as an array with same length as cat_thresh? How would you set that if hss_ec_value is used to compute both CTS (1 output line for each thresh) and MCTS (1 output line that includes all thresh) statistics?
2. Do I need to add more tests?

[j-opatz]

@JohnHalleyGotway given the complexity needed to make an hss_ec_value array work, I'm content with allowing 1 value only as it's currently used in MCTS. Otherwise, I would see the need to add logic that checks the length of cat_thresh and compares it to hss_ec_value and require it to either be the same length, or a length of 1. Length of 1 == mcts output, same length == cts output. But I can also see that going sideways quickly, or at the least becoming more complicated than it needs to be.

I will reach out to CPC for guidance on this.

For testing, I think what you already have in place is enough. Testing for the original intent of hss_ec_value seems enough to me, and you've already run tests to see if the new functionality works as expected (and it sounds like it does).

Some rapid-fire answers to questions you asked in the main Issue:

• No, I don't think EC_VALUE is needed in CTC or MCTC for that matter. The information seems better suited for CTS/MCTS.
• While we don't have anything to compare for this new method of calculating HSS, I was able to independently confirm that the hss_ec_value does behave as expected for MCTS output. CPC has already acknowledged that whatever MET produces for the CTS output with a constant hss_ec_value will be the new standard, as their old method did not properly account for misses.
• As noted above, I'm fine with keeping hss_ec_value to a single floating point value.

[JohnHalleyGotway]

@j-opatz I did find/fix this bug in the computation of bootstrap CI's for HSS_EC.

As we discussed elsewhere, the EC_VALUE column is needed in the CTC line type to enable the METplus Analysis suite (METviewer/calcpy/plotpy/express) to aggregate CTC lines together and recompute HSS_EC.

Please proceed with your review of this PR.

[j-opatz]

The document review and practical review of this PR seem to be in good shape. Unfortunately, there are no independent reviews/sources that can be directly compared to what's being done for CTS with this change: CPC's HSS values are still not quite in line with what's being done here, although the HSS value using the default, NA, is closer now.
The tests I've run show the expected behavior for increasing/decreasing the hss_ec_value, and the default value with "NA" is higher and closer to what CPC's original values indicated (as mentioned above). There may be the need to tweak the values that hss_ec_value can be set to (currently 0 to 1), but the logic of the PR is solid.