-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: a sympy backend #442
Conversation
ee3d79e
to
cd58f62
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #442 +/- ##
==========================================
+ Coverage 86.38% 86.77% +0.39%
==========================================
Files 96 98 +2
Lines 11139 11927 +788
==========================================
+ Hits 9622 10350 +728
- Misses 1517 1577 +60 ☔ View full report in Codecov by Sentry. |
14eebda
to
49dd9e5
Compare
1000b87
to
5253bad
Compare
0275233
to
82a2d47
Compare
TODO:
|
6b351cb
to
2c8b2db
Compare
Should finally be ready for a review! |
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.
All of this looks great! I made some high-level comments below, and I'm trusting the tests to ensure that the implementation is line-by-line correct.
I see that there are a lot more tests, but strangely, it's failing because Codecov says it's undershooting the target line coverage. I don't see how that's possible, considering that you added a few more classes and a lot more tests, including tests of non-SymPy functionality that wasn't tested before.
Is Codecov wrong? If not, is there a way to satisfy it with just a few more tests? If not, maybe we should just lower its threshold. Going a little under 85% is not terrible. Some lines of code will never be tested, like
else:
raise AssertionError(
f"unexpected case; should never get here {data = }"
)
@@ -54,6 +54,7 @@ dev = [ | |||
"pytest>=6", | |||
"pytest-cov>=3", | |||
"pytest-doctestplus", | |||
"sympy", | |||
] |
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.
It looks like SymPy is an optional dependency and that's good! ✔️
MomentumSympy2D, | ||
MomentumSympy3D, | ||
MomentumSympy4D, | ||
VectorSympy, |
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.
SymPy's preferred capitalization includes a capital "P", but we ignore NumPy's preferred capitalization, too. This is fine. ✔️
(MomentumSymPy4D
would be too bumpy!)
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
c27942c
to
4bc4407
Compare
Thank you for the review!
I am not sure how Codecov is processing coverage report here, but I'll try satisfying it for the lines that can be tested. I think the absolute coverage went up but the relative coverage went down. |
I'm just surprised because you added a lot more tests than code. I know that lines of test code don't correspond one-to-one with lines covered in the codebase, but it's still surprising to me. |
Added a few more tests, coverage went up! Merging! |
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.
Checklist
$ pre-commit run --all-files
or$ nox -s lint
)$ pytest
or$ nox -s tests
)$ cd docs; make clean; make html
or$ nox -s docs
)$ pytest --doctest-plus src/vector/
or$ nox -s doctests
)Before Merging