Skip to content

Conversation

@janden
Copy link
Collaborator

@janden janden commented Feb 17, 2022

This introduces some new tests in the spirit of #568. Since most of the tests (all except testElement) apply to both FBBasis2D and FFBBasis2D, these are structured as a mixin. There's probably a better way to do this, so I'm open to alternatives.

Apart from this structural issue, what remains to do here is

  • Remove the old tests and check that coverage doesn't go down.
  • Add a test to FFBBasis2D that does something similar to testElement for FBBasis2D.
  • Find some way to parameterize the tests over L and dtype, since all the new tests work for arbitrary values of these parameters.

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #576 (eb92614) into develop (11e9fc5) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #576      +/-   ##
===========================================
- Coverage    86.53%   86.52%   -0.02%     
===========================================
  Files          108      108              
  Lines         8287     8287              
===========================================
- Hits          7171     7170       -1     
- Misses        1116     1117       +1     
Impacted Files Coverage Δ
src/aspire/basis/fb_2d.py 98.47% <0.00%> (-0.51%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@janden janden force-pushed the new_fb2d_tests branch 3 times, most recently from 8e09642 to 6a6261b Compare February 17, 2022 18:28
@janden
Copy link
Collaborator Author

janden commented Feb 17, 2022

Coverage looks ok. We lose one line of coverage here:

v = v.asnumpy()
which is fine.

@janden
Copy link
Collaborator Author

janden commented Feb 18, 2022

Not super proud of how the tests are parameterized, but haven't found a better way to do it with the unittest framework.

@janden janden marked this pull request as ready for review February 18, 2022 07:48
@janden janden requested a review from j-c-c February 18, 2022 07:48
@janden janden mentioned this pull request Feb 18, 2022
Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. It will be nice to drop those hardcoded tests. I just have a few questions for now.

@janden janden requested a review from j-c-c March 8, 2022 17:41
@janden janden force-pushed the new_fb2d_tests branch 2 times, most recently from c11c92c to 3c1fc6d Compare March 10, 2022 19:55
@janden janden force-pushed the new_fb2d_tests branch 2 times, most recently from 8344a84 to 5a6d862 Compare March 31, 2022 17:13
Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Just have a couple questions.

@janden janden requested a review from j-c-c April 5, 2022 12:40
Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@janden janden merged commit f0671b5 into ComputationalCryoEM:develop Apr 7, 2022
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.

2 participants