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

TEST: fix medium test comparison #1688

Merged
merged 1 commit into from
May 9, 2024

Conversation

dmarek-flex
Copy link
Contributor

Resolved some issues with test that used exact comparisons. The default tolerances in np.isclose are suitable.

@dmarek-flex dmarek-flex self-assigned this May 8, 2024
@dmarek-flex dmarek-flex added the Bug something isnt working label May 8, 2024
@tylerflex tylerflex requested review from weiliangjin2021 and removed request for tylerflex May 8, 2024 17:38
@weiliangjin2021
Copy link
Collaborator

Thanks for fixing another float number issue! There are a few more exact comparisons in this test file. Change them all in this PR?

Copy link
Contributor

@marc-flex marc-flex left a comment

Choose a reason for hiding this comment

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

Thanks @dmarek-flex for fixing this!
I guess another question would be why it started complaining about this now. Like, what changed?

tests/test_components/test_custom.py Outdated Show resolved Hide resolved
@dmarek-flex
Copy link
Contributor Author

dmarek-flex commented May 8, 2024

I guess another question would be why it started complaining about this now. Like, what changed?

Well part of the test relies on a random number generation, which maybe is why it happens from time to time. Although there seems to be a constant seed used. Over the day I have been getting both passes and fails. I don't know why that would not show up on the github tests though. And then on one of the versions of the test some interpolation is used, which could result in some introduced error I guess.

@dmarek-flex dmarek-flex force-pushed the dmarek/fix_medium_test_comparison branch from c9c1a54 to 6a43c71 Compare May 9, 2024 14:11
@dmarek-flex
Copy link
Contributor Author

I ended up using a relative tolerance which is equal to single precision epsilon. Other tests would fail if I tried to be very strict with double precision epsilon. In lieu of choosing a relative tolerance for each comparison, I just chose to relax the tolerance.

@dmarek-flex
Copy link
Contributor Author

@marc-flex It would be helpful if you could checkout this branch yourself and see if any test fails.

@marc-flex
Copy link
Contributor

@marc-flex It would be helpful if you could checkout this branch yourself and see if any test fails.

Of course! :) I just checked and all test passed

@dmarek-flex dmarek-flex merged commit 169b1c0 into pre/2.7 May 9, 2024
15 of 16 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/fix_medium_test_comparison branch May 9, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug something isnt working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants