Feature 3014 sfs updates #3020
Feature 3014 sfs updates #3020
Conversation
|
At the moment, GHA is not finding nearly 60 files that were previously in the truth data for use case 0, but are no longer in the branch. This is a concern that should be investigated. There is a similar issue happening with use case 1, but on a much larger scale (186!). However, this might not be a real issue: it seems like the previous truth data was stored under one directory structure, while the current branch has a new directory structure. 186 is an evenly divisible value and it seems like for every "missing" truth file there is a "new" file. I checked two or three file names and found the "missing" warning, along with a "new" warning to confirm my suspicion, but you should confirm that's the cause of the difference (rather than hand checking all 93 files). Once I have confirmation of the reasons for these changes, I'll reassess the PR and approve. |
|
@j-opatz yes. I had to change the output location for the command line plotting in case 1. At this time, command line plotting doesn't have support for files to be in different directories (such as date directories). So, I'm expecting that it will not find all of those files, but then there should be a bunch of new output files. For case 0, I tweaked the grid slightly. I didn't think the output directory had changed but I'll double check this. |
|
@CPKalb any word on those checks for case 0? It's a bit odd that a grid update would cause so many missing files. That's more of a behavior where the use case had an updated path (although it should find new files like case 1), or removed a feature/the use case is no longer running properly. I took a look at the rest and only had two remaining comments:
I did not complete any additional steps for the review, such as updating Mohawk to use the new data. Once you've confirmed that case 0's missing files in GHA is just a result of changed grids, I can finish the Mohawk data update and approve the PR. |
|
I just returned from travel so haven't even had the chance to look yet into what is going on with the files. There are no images for case 0, but case 1 should have output images. If it doesn't, then that's and error of some kind. But I'll take a look tomorrow and get this resolved. |
Just a clarification on this: I had meant images for the use case thumbnail, not the images that the use case produces. However, you can always use the output images as the thumbnail! |
|
@j-opatz, I found the issue. It looks like a setting I had updated for training made it's way into the pull request erroneously. I fixed this setting, and in the new GitHub actions testing, I'm not seeing anymore missing files. I'm just seeing differences, which is expected, since I made a slight change to the grid. Is this what you are seeing as well? https://github.com/dtcenter/METplus/actions/runs/15862200953 Tina |
|
@j-opatz, I see use case thumbnails for both cases when I display the documentation. Are you not able to see them? |
* Adding graphic * Updating python embedding script location * Updating environment * Updating environment * Removing bad path * Adding CI override * Updating documentation * Updating documenttion again * Take 3 * Another documentation update * Updating use case * Updating the order of processes * Updating documentation * Cleaning up unused variables * Updating documentation * Adding climatology. Testing will fail b/c haven't uploaded data * Adding Series Analysis climatology and updating documentation * Another doc update * Fixing valid_beg * fixing time * Fixing documentation typo * Fixed another typo * Fixing log verbosity and block size * Updating some code * Fixing typo * Testing removal of file_type * Removing file type * Setting tests to false for pull requests * minor grammar, spelling updates * Fixing increment * Fixing Log level * Update GridStat_fcstSFSGSL_obsERA5Land_SoilMoisture.conf * editing plot title --------- Co-authored-by: Christina Kalb <kalb@seneca.rap.ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
* Adding graphic * Updating python embedding script location * Updating environment * Updating environment * Removing bad path * Adding CI override * Updating documentation * Updating documenttion again * Take 3 * Another documentation update * Updating use case * Updating the order of processes * Updating documentation * Cleaning up unused variables * Updating documentation * Adding climatology. Testing will fail b/c haven't uploaded data * Adding Series Analysis climatology and updating documentation * Another doc update * Fixing valid_beg * fixing time * Fixing documentation typo * Fixed another typo * Fixing log verbosity and block size * Updating some code * Fixing typo * Testing removal of file_type * Removing file type * Setting tests to false for pull requests * minor grammar, spelling updates * Fixing increment * Fixing Log level * Update GridStat_fcstSFSGSL_obsERA5Land_SoilMoisture.conf * editing plot title --------- Co-authored-by: Christina Kalb <kalb@seneca.rap.ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
* Adding graphic * Updating python embedding script location * Updating environment * Updating environment * Removing bad path * Adding CI override * Updating documentation * Updating documenttion again * Take 3 * Another documentation update * Updating use case * Updating the order of processes * Updating documentation * Cleaning up unused variables * Updating documentation * Adding climatology. Testing will fail b/c haven't uploaded data * Adding Series Analysis climatology and updating documentation * Another doc update * Fixing valid_beg * fixing time * Fixing documentation typo * Fixed another typo * Fixing log verbosity and block size * Updating some code * Fixing typo * Testing removal of file_type * Removing file type * Setting tests to false for pull requests * minor grammar, spelling updates * Fixing increment * Fixing Log level * Update GridStat_fcstSFSGSL_obsERA5Land_SoilMoisture.conf * editing plot title --------- Co-authored-by: Christina Kalb <kalb@seneca.rap.ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Pull Request Testing
Describe testing already performed for these changes:
Ran the use case to verify that it works and produces output that looks reasonable.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Run the two updated soil moisture use cases (GridStat_fcstSFSGSL_obsERA5Land_SoilMoisture and PcpCombine_obsERA5_obsOnly_soilMoisturePreProcessing) and verify that the results match what is expected. Both of these use cases are in the s2s_soil_moisture directory. Expected results are stored on seneca in /d1/personal/kalb/SFS/SoilMoisture/output/s2s_soil_moisture
Also, take a look at the documentation for GridStat_fcstSFSGSL_obsERA5Land_SoilMoisture and provide any feedback.
The documentation for PcpCombine_obsERA5_obsOnly_soilMoisturePreProcessing did not change since the use case only had a small update to the output grid. Review of this documentation is optional, as it was reviewed when the use case was first added to the repository.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes or No]
Yes
Do these changes include sufficient testing updates? [Yes or No]
I'm not sure what this means.
Will this PR result in changes to the test suite? [Yes or No]
If yes, describe the new output and/or changes to the existing output:
Yes. There will be new output for the GridStat use case and updated output for the PcpCombine use case. Current testing should show differences in the truth data for both of these use cases. Also, there was new input data for the GridStat use case, but not for the PcpCombine use case.
Do these changes introduce new SonarQube findings? [Yes or No]
If yes, please describe:
Yes. There are complaints about duplicated code for reformat_CNT_linetype.py and plot_line_stats.py. Previous discussion with George McCabe suggest that at some point in the future, we should convert these scripts for command line reformatting and plotting to be wrappers, since similar code will likely be used in any use case with command line plotting. However, until we have the funds for this work, these scripts will exist as mostly duplicated code in the repository and will cause SonarQube findings.
Please complete this pull request review by [Fill in date].
Before the 6.1 release. If you are using the DTC storm funds for this review, then before those funds expire on June 30.
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or METplus-Wrappers-X.Y.Z Development project for official releases