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

Removes top-level tshirt models #597

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

mattw-nws
Copy link
Contributor

@mattw-nws mattw-nws commented Aug 2, 2023

Builds on/requires #595

Removes tshirt, tshirt_c, associated data and test files, and the Bmi_C_Cfe_IT test (which depended on tshirt).

Additions

  • None

Removals

  • tshirt, tshirt_c, associated data and test files, and the Bmi_C_Cfe_IT test (which depended on tshirt).

Changes

  • CMakeLists.txt removals for the legacy components, e.g. the compare_cfe target is now gone.

Testing

Screenshots

Notes

Todos

  • Documentation and example file revisions will wait till the end of the PR chain

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • Linux

@PhilMiller
Copy link
Contributor

This looks good to me once the predecessors are merged and this gets rebased.

@mattw-nws
Copy link
Contributor Author

It is worth pointing out that these legacy formulations (including LSTM) are

@mattw-nws mattw-nws closed this Aug 7, 2023
@mattw-nws mattw-nws reopened this Aug 7, 2023
@mattw-nws
Copy link
Contributor Author

It is worth pointing out that my trackpad is garbage.

@mattw-nws
Copy link
Contributor Author

It is worth pointing out that these legacy formulations (including LSTM) are responsible for the essentially broken behavior of all current forcing providers that implement get_data_start_time and get_data_end_time --they are given and provide back the simulation start and end time and nothing to do with the forcing data. The legacy formulations were (are) getting their simulation start and end times this way, so it couldn't be changed to more reasonable behavior. Now this is somewhat of a sticking point in #601 , or at least there's not a clean way to fix Bmi_Multi_Formulation's use of the same methods. Just one more reason to do this.

@PhilMiller
Copy link
Contributor

I'm +1 once 595 is merged (and hence 594 before it)

robertbartel
robertbartel previously approved these changes Aug 7, 2023
…t is. This should probably be a workflow test not a GoogleTest, and it shouldn't test the results against another piece of code inside the project--it looks more like this is testing tshirt, not the other way around, or at least it's not clear which is being tested--this should probably be a test against known static values. Plus, it requires tshirt and probably the et_calc stuff, which we're trying to get down to removal of.
@mattw-nws mattw-nws merged commit 7645eb5 into NOAA-OWP:master Aug 8, 2023
20 checks passed
@mattw-nws mattw-nws mentioned this pull request Aug 8, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants