-
Notifications
You must be signed in to change notification settings - Fork 35
fixing sympy tests #581
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
fixing sympy tests #581
Conversation
Saransh-cpp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @Schefflera-Arboricola! The PR raises the question of structural (x == y) v/s symbolic (simplify(x - y) == 0 or Eq(x, y)) equality. Even though symbolic equality would be easier to maintain in the longer run, it would be better to use the structural equality wherever possible to be more strict as -
- we test (or at least, aim to test when we drop Python 3.8 support (uncompyle6 does not support Python 3.9+)) the ability of vector's compute functions to operate only on data containers using the SymPy tests - https://github.com/scikit-hep/vector/blob/main/tests/test_compute_features.py
- the SymPy tests don't fail often, so the maintainability is not that big of a problem
See my comment below to address this.
|
Tests failing without being mathematically wrong is bad IMO, since they will sooner or later fail with sympy updates (as is the case now from 1.13.1 -> 1.14.0). Hence I prefer symbolic comparison. In fact it is quite similar to checking floating points by an epsilon range, since the backend computation can change, but the result is still right (wrt. the scope). |
|
Thanks, @APN-Pucky! My comment behind using structural equality was because the "symbolic" nature of our compute functions will solely be tested by SymPy once Python 3.8 is removed from GH Actions; hence, we would want these tests to be strict (unless we find another way - #531). But at the same time, I don't think this strictness is actually adding anything to the test suite - the symbolic equality will test the exact same thing but in a more flexible way. Given that we use structural equality everywhere in the tests, I have reverted to keep using that for now. However, I will open an issue to discuss this further (and possibly change all equalities in the tests in the future). |
Saransh-cpp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Schefflera-Arboricola! I'll merge this to fix the tests.
|
Thank you @Saransh-cpp ! |
Description
Kindly take a look at CONTRIBUTING.md.
Please describe the purpose of this pull request. Reference and link to any relevant issues or pull requests.
resolves #579
Failure messages from CI : https://github.com/scikit-hep/vector/actions/runs/14703378893
Failure messages local runs
Checklist
$ pre-commit run --all-filesor$ nox -s lint)$ pytestor$ nox -s tests)$ cd docs; make clean; make htmlor$ nox -s docs)$ pytest --doctest-plus src/vector/or$ nox -s doctests)Before Merging