Refactor fmpz_mpoly and fmpq_mpoly arithmatic functions#190
Refactor fmpz_mpoly and fmpq_mpoly arithmatic functions#190oscarbenjamin merged 1 commit intoflintlib:masterfrom
Conversation
Jake-Moss
changed the title
Jake-Moss
force-pushed
the
mpoly_update
branch
from
b16f140 to
5f80eff
Compare
|
This is now rebased on master |
| def any_as_scalar(self, other): | ||
| if isinstance(other, int): | ||
| return any_as_fmpq(other) | ||
| elif typecheck(other, fmpz): | ||
| return any_as_fmpq(other) | ||
| elif typecheck(other, fmpq): | ||
| res = fmpq.__new__(fmpq) | ||
| fmpq_set((<fmpq>res).val, (<fmpq>other).val) | ||
| return res |
There was a problem hiding this comment.
If it is an fmpq already can we not just return it?
Why does a copy need to be made?
There was a problem hiding this comment.
I thought it best to be consistent with the other any_as_* methods, but since they are immutable on a python-flint level perhaps we can just return the same object.
any_as_fmpq is just not used there because no conversion is required
>>> from flint import *
>>> x = fmpz(1)
>>> y = any_as_fmpz(x)
>>> x is y
False(With any_as_fmpz modified to be a cpdef)
There was a problem hiding this comment.
Maybe any_as_fmpz should be changed to not make a copy. I'm not sure that is compatible with how it is currently used though like whether the object returned is ever mutated.
|
|
||
| def __pow__(self, other, modulus): | ||
| def _add_mpoly_(self, other: fmpq_mpoly): | ||
| cdef fmpq_mpoly res |
There was a problem hiding this comment.
Maybe these methods should all be cdef.
There was a problem hiding this comment.
I've just tested this locally and it's possible, just needed a little massaging due to C-methods not being visible at runtime. The non-commutative operands can't use runtime reflection after coercion anymore, it needs a explicit C reflected function e.g. _rtruediv_mpoly_ which can just call the normal C _truediv_mpoly_ after a cast + argument swap. Will have this up this evening
There was a problem hiding this comment.
Actually should just be able to use the normal dunder methods after the coercion rather than have a reflected cdef. Though using the cdefs all the way down is probably slightly faster.
|
This looks good. Improvements can always be made later. |
2 participants
This refactors fmpz_mpoly and fmpq_mpoly to use the new mpoly structure added in #164. It is based on #189, I'll rebase this PRs one real commit once #189 is merged