-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
sf fixes #37976
sf fixes #37976
Conversation
…; moving towards full coverage.
… of cache may contain variable, add failing test
….com/mantepse/sage into lazy_series/integration_experimental
…reams which are different
…s, use 'is' for equality when finding input streams in Stream_uninitialized
….com/mantepse/sage into lazy_series/integration_experimental
S = R.base_ring()[tuple(Z)] | ||
S = PolynomialRing(R.base_ring(), Z) |
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.
I think that using __getitem__
to define a polynomial ring in library code is generally a very bad idea. I found a few other occurrences with a very naive grep, possibly one could find more by inserting a print statement in __getitem__
and running the test suite (and work very hard). @fchapoton, do you agree?
grep -r --include=*.{py,pyx} --color -nH --null -e "ring\[" *
grep -r --include=*.{py,pyx} --color -nH --null -e "R\['" *
Here goes:
algebras/hecke_algebras/cubic_hecke_algebra.py:962: pol_bas_ring = base_ring['h']
combinat/finite_state_machine_generators.py:1276: polynomial_left = base_ring[var](left_side.operands()[0])
combinat/finite_state_machine_generators.py:1331: polynomial_right = base_ring[var](next_function.operands()[0])
dynamics/arithmetic_dynamics/projective_ds.py:5769: T = base_ring['k']
interfaces/sympy.py:195: return base_ring[variables]
interfaces/sympy.py:227: R = base_ring[variables]
matrix/matrix_space.py:1111: return self.__change_ring[R]
matrix/matrix_space.py:1117: self.__change_ring[R] = M
matrix/matrix2.pyx:3675: R = self._base_ring[var] # polynomial ring over the base ring
modular/quasimodform/ring.py:268: self.__polynomial_subring = self.__modular_forms_subring[name]
schemes/elliptic_curves/hom_velusqrt.py:888: R, Z = self._internal_base_ring['Z'].objgen()
schemes/elliptic_curves/hom_velusqrt.py:1134: S = self._internal_base_ring['x,y']
schemes/elliptic_curves/hom_velusqrt.py:1171: S = self._internal_base_ring['x']
schemes/elliptic_curves/ell_field.py:1604: return R['x'].one()
schemes/elliptic_curves/ell_generic.py:3399: R = RR['x']
schemes/elliptic_curves/hom_velusqrt.py:902: V = R['V'].gen()
dynamics/arithmetic_dynamics/projective_ds.py4575: T = R['t']
dynamics/arithmetic_dynamics/projective_ds.py4932: T = R['t']
functions/transcendental.py633: f = PolynomialRealDense(R['x'], coeffs)
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.
I definitely agree. It is syntactic sugar. Let’s change all of these in the library.
Should I laugh or cry?
Update 1: apparently, the last line is equivalent to
but I couldn't find the implementation yet. Update 2:
|
Documentation preview for this PR (built with commit 04b6abc; changes) is ready! 🎉 |
Can we also separate this from #37033 or is there something explicitly in there that this needs? |
In principle yes, although my main goal is to fix #37975. So, I thought of this as being a "meta-PR". I would actually like to factor out individual commits fixing separate things, once I know what to do. I am currently stuck with the negation issue mentioned above, and just asked for help on sage-devel. |
With the following experimental change, I am getting a little further. diff --git a/src/sage/modules/free_module_element.pyx b/src/sage/modules/free_module_element.pyx
index c93c71f195e..c885f1e82c7 100644
--- a/src/sage/modules/free_module_element.pyx
+++ b/src/sage/modules/free_module_element.pyx
@@ -4967,7 +4968,8 @@ cdef class FreeModuleElement_generic_sparse(FreeModuleElement):
cdef dict v = {}
if right:
for i, a in self._entries.iteritems():
- prod = (<RingElement>a)._mul_(right)
+ prod = a._mul_(right)
if prod:
v[i] = prod
return self._new_c(v) |
So, here is where I am now failing:
|
Note that
where If If the parent of v really is a two-sided module, something goes wrong in the action discovery there. |
The last commit makes things work in the specific doctest (labelled Dyck paths Frobenius character), but I am not sure whether this is (even in principle) the correct fix. Also, I think we should be more careful with respect to empty sums in |
superseded by #38429 |
Fix #37975
Depends on #37033