Merged
Conversation
joelridden
reviewed
Aug 25, 2024
| dbottom: float, | ||
| length: float, | ||
| width: float, | ||
| projected_width: float, |
Contributor
There was a problem hiding this comment.
docstring should also be updated
Contributor
Author
There was a problem hiding this comment.
In the long-term, a better fix would be to take dip + width as an argument and then compute projected width in the function. But considering that you do make use of this code I will refrain from changing the API immediately.
joelridden
approved these changes
Aug 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds extensive test cases to the source modelling repository. With this pull request merged, every current module will be tested. This testing effort is a part of my overall workflow testing strategy, and part of the type-5 validation (and general code validation efforts).
You can ignore the files in the
srfsandfspsdirectory, they just contain test files.Testing Strategy
We currently have a very ad-hoc testing strategy. We test some code half-heartedly, and only where the testing is easy to do. This is ok for low-risk code, but ideally anything involved in Cybershake should be thoroughly tested since the cost of bugs in a Cybershake run can be really high. I want to change this as much as is feasible (recognising that some repositories are too hard to test properly). Broadly, I have identified three testing strategies that I think can enable high quality testing of the different kinds of code we write:
This kind of testing is suitable where we can easily validate properties that must hold, and where generation of test cases is simple. For example, the
qcore.bounding_boxmodule could have hypothesis testing to check that (a) the bounding box contains the set of points it is supposed to bound, and (b) it is smaller than a random sampling of other boxes that contain the points.srfandfspparsers are not amenable to hypothesis testing because it's really hard to generate valid files.However we can unit test to check that certain files parse correctly/fail, and verify their properties.
The Testing GitHub Action
The GitHub action included in this pull request checks that all the tests pass and that every module has 95% test coverage. Going forward, a pull request to the source modelling repo cannot be merged without the code coverage for each module staying above 95% and without passing test cases.