-
-
Notifications
You must be signed in to change notification settings - Fork 692
Replaced all instances of <RingElement> with <Element> #40648
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
Conversation
|
|
given that in one of the run the standard deviation of 10⁶ runs is 8ns, and the difference is on the order of 20ns, I'd be tempted to dismiss it as random fluctuation. This difference is… around 2.5σ, so roughly p=0.012? |
|
do add a doctest. |
It is probably happening in each loop of the run and in each run right? One sure shot method to verify is to take a larger dimension say 100 or 1000 and do performance testing then. |
|
I added a doctest that uses the original code that generated the error. Is the location where I put it ok? |
|
|
||
| sage: R = cartesian_product([ZZ, ZZ]) | ||
| sage: assert R in CommutativeRings() | ||
| sage: matrix(1, 1, [R.zero()]) * vector([R.zero()]) |
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.
Consider using lmul or rmul explicitly. We can have both tests - matrix * vector and vector * matrix.
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.
by lmul/rmul you likely mean testing multiply the matrix on left/right side. Calling the internal method explicitly is fine, but not possible if the method is cdef only.
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.
The method being tested is cpdef _lmul_(self, Element right):, thus I believe both matrix * vector and matrix._lmul_(vector) should be tested, similar to other tests above.
| cdef list a = left._entries | ||
| cdef list b = (<FreeModuleElement_generic_dense>right)._entries | ||
| v = [(<RingElement> a[i])._add_(<RingElement> b[i]) for i in range(left._degree)] | ||
| v = [(<Element> a[i])._add_(<Element> b[i]) for i in range(left._degree)] |
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.
A test for v + v should be added as well, probably similarly other operations can also be tested for.
|
while adding the tests for other operations (+, etc.) is fine, I think the risk of regression is very low. Now wait for someone to approve workflows. |
|
@tscrim requesting workflow approval on this PR |
|
Documentation preview for this PR (built with commit 1bb74cc; changes) is ready! 🎉 |
Fixes #40611
Performance Measurement
Before
After
📝 Checklist
⌛ Dependencies