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

Bugfix/shape space test #52

Merged
merged 6 commits into from
Mar 22, 2024
Merged

Bugfix/shape space test #52

merged 6 commits into from
Mar 22, 2024

Conversation

pgarrison
Copy link
Collaborator

@pgarrison pgarrison commented Mar 21, 2024

Summary

Adding tests for shape mode calculations used by NucMorph. In the process I found a bug cause by the typo get_Lmax (should be get_lmax).

Changes

  • Fixed typo
  • Three new tests.
  • The tests have some basic output validation. In addition, I used a fixed RNG seed and saved the expected outputs to the tests/data folder to compare across refactors.

Testing

I ran pytest and all three tests pass. From the commit before the bugfix, one test fails because of the get_Lmax bug.

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • N/A Link to any relevant issue in the PR description. Ex: Resolves [Adjusted the general and cluster module to be more general #12], adds tiff file format support
  • Provide context of changes.
  • Provide relevant tests for your feature or bug fix.
  • N/A Provide or update documentation for any feature added by your pull request.

@pgarrison pgarrison requested a review from vianamp March 21, 2024 19:00
assert df[column].dtype == np.float64
numzero = sum(df[column] == 0.0)
# Since we used gaussian data the result should almost always have no zeros
assert numzero < 3

Choose a reason for hiding this comment

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

If there should almost always be no zeros why are we asserting that there is less than 3? If its an almost should we even test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's similar to the probability of picking a random float and seeing if it happens to be an integer, a very tiny probability. The probability that 3 of the values are exactly zero is exceptionally tiny. Error cases would look like having a whole column that's all zero.

I suspect there is a clearer way to write this but I don't have a great idea. It could just be assert numzero == 0.

cvapipe_analysis/tests/test_shape_space.py Outdated Show resolved Hide resolved
cvapipe_analysis/tests/test_shape_space.py Outdated Show resolved Hide resolved
return df


def assert_NUC_PC_columns_nonzero(df):

Choose a reason for hiding this comment

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

All other test methods have, Arrange, Act, and Assert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a test, just a helper function

@pgarrison
Copy link
Collaborator Author

Apparently the test did not pass! Since I copied the wrong summary.html. Fixed in f09d5ba.

I'll also try to set up github actions to block merging if the tests don't pass

@pgarrison
Copy link
Collaborator Author

CI failure is due to broken CI config: future work

@pgarrison pgarrison merged commit 97e610d into master Mar 22, 2024
@pgarrison pgarrison deleted the bugfix/shape-space-test branch March 22, 2024 18:43
pgarrison added a commit that referenced this pull request Mar 22, 2024
This was referenced Mar 26, 2024
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.

4 participants