Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

This is happening because the Rotation class crashed for me when it tried to print/log itself (fixed elsewhere)... so besides fixing that I want to check all the class's methods are at least invoked by pytest. I think some of the more advanced calls were already covered with some scrutiny. I didn't run into any other crashes yet.

One concern, is whether there is a more robust way to implement testCommonLines s.t. we can easily change num_rots at will. Flexing num_rots by hand I found one other flaky test (up to atol), but I can imagine other reasons that might cause this particular testCommonLines result to change over time...

@garrettwrong garrettwrong added the CI Continuous Integration label Mar 10, 2021
@garrettwrong garrettwrong self-assigned this Mar 10, 2021
@garrettwrong garrettwrong requested a review from janden as a code owner March 10, 2021 17:18
@janden
Copy link
Collaborator

janden commented Mar 11, 2021

One concern, is whether there is a more robust way to implement testCommonLines s.t. we can easily change num_rots at will. Flexing num_rots by hand I found one other flaky test (up to atol), but I can imagine other reasons that might cause this particular testCommonLines result to change over time...

So the issue here is that the common lines are hardcoded, which means we can't change rot_obj without breaking things? One simple thing would be to keep a separate rotation stack just for testCommonLines. With a little work, we can even construct it so that we know what the common lines should be without having to hardcode them.

@garrettwrong
Copy link
Collaborator Author

Yes, the common lines are hard coded, cherry picked from the results running at num_rots=32 with a specific seed. Under any other circumstance, or potentially a dependent package upgrade etc etc, the test would probably fail.

The first option (rots specific to the common lines test) is easy, but the right thing to do is probably the second option, by construction... but I would have to think on that for a while. Ideally this code might be run with num_rots parameterized and fed at least two different num_rots... But this isn't a priority.

@garrettwrong garrettwrong merged commit 904846c into develop Mar 11, 2021
@garrettwrong garrettwrong deleted the issue_396 branch March 11, 2021 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous Integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants