-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
special subs for fractions #37143
special subs for fractions #37143
Conversation
@tscrim, the current pull request has a strange problem, when
Experimentally, I replaced |
AFAICS, For Sorry I cannot work much more on this due to preparing to travel back from Okinawa to Sapporo tomorrow. I will try to get to this (and the other PR) soon. |
Hm. I only see as signatures either
It would be good to know what the intended signature is.
No pressure! |
I believe the "correct" signature is the first argument is the I think the best fix is to follow what This makes the |
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 seems like that also worked for you. Just a few minor things. Although I would probably remove the INPUT and OUTPUT blocks as they provide no useful information.
The failure in |
This seems to be a general issue too. Before, That being said, it probably would be better to be more strict about this input keys being convertible (perhaps coerceable?) into the defining ring of the fraction field. Nor can we assume the inputs are in ta polynomial ring. Thus, I think your first option is best: check if the first argument of |
…merator and denominator, unify subs signature, allow generator indices as keys consistently
I think this is a step backwards for fraction field elements. Specifically, fraction fields are not just for polynomials; they should not necessarily require Definitely +1 on the change to
Also, if |
I think I don't understand yet what you are writing and what you would like to have. Let me try to explain my intentions and how I understand the current situation in sage mainline. The philosophy I applied is that The generators of the fraction field are the generators of the underlying ring, turned into elements of the fraction field. I don't understand what you have in mind concerning the signature, I thought there are only three different ways to call 1.) either passing a dict (which should be the preferred option in my opinion) For 1.) there is currently at least one constructor that allows the keys to be integers, the indices of the generators instead The first example you provide still works, essentially because subs only sees the result of (I'm here at #sd125, best wishes from Frederic who is sitting in the sun next to me.) |
Ah, sun; here in Hokkaido the sun means icy walkways... Let me try to expand a bit on my concern. Note in vanilla Sage:
Actually, this seems to be inconsistent with the multivariate case. So big +1 on allowing. So my first "example" for the univariate case works but not for the multivariate:
Note that I would say the (2) is the most natural, and the infinite poly ring should handle it properly. Although that is a separate discussion. Looking forward to seeing both of you in March. |
I think I understand your concern, but I would not know what to do with I must translate the generators passed in a dict to the underlying ring, I don't see a way out of that. (I would also rather not use coercion to do that. If I understand correctly, we agree here). Of course, I could check for each argument whether it is a dictionary, and then replace the keys of these. Is this what you have in mind? I don't see the benefit, I must say. I might also misunderstand you because you said before that Frédéric just noticed that in
However, I do not see the benefit of this signature over
So to summarize, I need to know what to do if |
You would have def subs(in_dict=None, *args, **kwds):
if isinstance(in_dict, dict):
# parse
num = self._numerator.subs(in_dict, *args, **kwds)
You actually are secretly using coercion by
That's not quite what I said. Just that
You can just pass them along. |
if not in_dict: | ||
return self(*args, **kwds) |
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.
Note that this is now a subtle change in behavior. Before, the empty dict {}
would perform no substitutions if extra arguments were passed. Now it will do a substitution based on the inputs. The behavior is not well-defined in either case (mixing two input methods), but this will now agree with the multivariate case. I am just pointing this out this behavior change.
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 agree. Note that, a priori, subs
is not disallowing mixing of substitution methods:
sage: R.<x,y> = QQ[]
sage: p = x + y
sage: p.subs({0: 1}, y=2)
3
def to_R(m): | ||
try: | ||
mi = gens.index(m) | ||
except ValueError: | ||
return m | ||
return mi | ||
in_dict = {to_R(m): v for m, v in in_dict.items()} |
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.
This can be stronger than doing conversion (quite possibly more expensive too) but it could also be independent. The issue is that equality check that is done as part of index()
goes through coercion. This means that it could potentially construct a pushout and do the comparison in the larger ring. So it might allow for unrelated variables than a strict conversion R(m)
check would not allow. On the other than, conversion can be much more general depending on the implementation, but I would imagine it covers most reasonable pushout cases that would result in an equality. It is likely to be the most consistent as well.
Also, this implementation might rely on equal elements having equal hashes (which Python says should be true, but coercion and pushouts can lead to messy cases where this unintentionally does not hold).
Because of this, I think you are best simply doing
R = self.parent()
in_dict = {R(m): v for m,. v in in_dict.items()}
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 we want to restrict keys to generators and indices of generators. I think your suggestion does not allow indices, although that should be easy to fix. However, I would also like to look at a few concrete examples to make up my mind. My experience with the constructors is that they are quite careless in some cases, which is why I prefer to avoid them.
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.
Sorry, I was very imprecise and misleading. What I meant to say is, that we want to restrict keys to things that the user regards as generators. More to the point: I think we don't want to allow for things like {x/y: 1}
in a ring of rational functions. On the other hand, maybe we should, I don't really know.
The idea to allow integers is to have a possibility of speedup: if the key is an integer, it should be possible to avoid expensive operations completely. I am well aware of the fact that the current code does not take advantage of this possibility.
It surprises me that conversion might be more consistent than checking equality. Are you sure about that?
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.
To summarize: would you think
R = self.parent()
in_dict = {m if isinstance(m, (Integer, int)) else R(m): v for m, v in in_dict.items()}
is better?
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.
Yes, that is good after a slight variant:
R = self.parent()
in_dict = {ZZ(m) if m in ZZ else R(m): v for m, v in in_dict.items()}
to catch things like {2/2: x+y}
. My previous proposal, in principle, does allow for integers, but it would break over finite fields in a subtle way.
With regards to your other comments, things like {x/y: 1}
would fail, but perhaps at a later stage when it compares with the generators. For what a user looks like a generator can be very broad. Consider the following example that generates a pushout:
sage: R.<x,t> = QQ[]
sage: S.<x> = ZZ['a'][]
sage: x + t
x + t
sage: _.parent()
Multivariate Polynomial Ring in x, t over Univariate Polynomial Ring in a over Rational Field
sage: S.gen() == R.gen(0)
True
Is the generator of S
a generator of R
and vice versa? The conversion does work in this case. However, what about the string 'x'
?
To put it another way, I think each ring should be responsible for what it considers as input and is the best thing to decide that. Yes, some are imprecise/not-well-coded, but the specifications about what is an element (and hence might be a generator) is more clear.
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.
Unfortunately, this does not work:
sage -t --warn-long 81.0 --random-seed=222573162246150205167095728432903358574 src/sage/rings/fraction_field_element.pyx
**********************************************************************
File "src/sage/rings/fraction_field_element.pyx", line 374, in sage.rings.fraction_field_element.FractionFieldElement.__hash__
Failed example:
((x+1)/(x^2+1)).subs({x:1})
Exception raised:
Traceback (most recent call last):
File "/home/martin/sage-trac/src/sage/doctest/forker.py", line 712, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/martin/sage-trac/src/sage/doctest/forker.py", line 1147, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.rings.fraction_field_element.FractionFieldElement.__hash__[15]>", line 1, in <module>
((x+Integer(1))/(x**Integer(2)+Integer(1))).subs({x:Integer(1)})
File "sage/rings/fraction_field_element.pyx", line 481, in sage.rings.fraction_field_element.FractionFieldElement.subs
num = self._numerator.subs(in_dict, *args, **kwds)
File "sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 3559, in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.subs
raise TypeError("keys do not match self's parent")
TypeError: keys do not match self's parent
**********************************************************************
File "src/sage/rings/fraction_field_element.pyx", line 474, in sage.rings.fraction_field_element.FractionFieldElement.subs
Failed example:
p.subs({v: 100})
Exception raised:
Traceback (most recent call last):
File "/home/martin/sage-trac/src/sage/doctest/forker.py", line 712, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/martin/sage-trac/src/sage/doctest/forker.py", line 1147, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.rings.fraction_field_element.FractionFieldElement.subs[7]>", line 1, in <module>
p.subs({v: Integer(100)})
File "sage/rings/fraction_field_element.pyx", line 481, in sage.rings.fraction_field_element.FractionFieldElement.subs
num = self._numerator.subs(in_dict, *args, **kwds)
File "sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 3559, in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.subs
raise TypeError("keys do not match self's parent")
TypeError: keys do not match self's parent
**********************************************************************
2 items had failures:
1 of 32 in sage.rings.fraction_field_element.FractionFieldElement.__hash__
1 of 9 in sage.rings.fraction_field_element.FractionFieldElement.subs
[295 tests, 2 failures, 0.72 s]
----------------------------------------------------------------------
sage -t --warn-long 81.0 --random-seed=222573162246150205167095728432903358574 src/sage/rings/fraction_field_element.pyx # 2 doctests failed
----------------------------------------------------------------------
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 am guessing that this is because the other instances of subs
use equality, not conversion. But I don't know.
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.
No, they aren’t unless I am missed something. Sorry, I forgot where this was supposed to live; it should have been R = self.parent().base()
as it should be a polynomial ring (more generally, the defining domain) generator.
Documentation preview for this PR (built with commit 3bd05ab; changes) is ready! 🎉 |
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.
Thank you. LGTM. Sorry that this ended up being a bit more complicated.
At some point, we probably should also try to improve the subs
for multivariate polynomials. It seems like we could some speed out...
sagemathgh-37143: special subs for fractions We create a specialised method `subs` for `FractionFieldElements`, for better performance. In particular, the generic `Element.subs` method insists on replacing all gens, which is a problem in polynomial rings with many variables, such as (potentially) the `InfinitePolynomialRing`. Fixes sagemath#37122. URL: sagemath#37143 Reported by: Martin Rubey Reviewer(s): Martin Rubey, Travis Scrimshaw
sagemathgh-37210: unify alias substitute for subs We unify the definitions as follows: * `substitute` is a method in `Element` which calls `self.subs` and passes all arguments along, and not defined anywhere else. * `subs` is the method that should be overwritten in the various element classes. Dependencies: sagemath#37143 URL: sagemath#37210 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton
sagemathgh-37210: unify alias substitute for subs We unify the definitions as follows: * `substitute` is a method in `Element` which calls `self.subs` and passes all arguments along, and not defined anywhere else. * `subs` is the method that should be overwritten in the various element classes. Dependencies: sagemath#37143 URL: sagemath#37210 Reported by: Martin Rubey Reviewer(s): Frédéric Chapoton
We create a specialised method
subs
forFractionFieldElements
, for better performance. In particular, the genericElement.subs
method insists on replacing all gens, which is a problem in polynomial rings with many variables, such as (potentially) theInfinitePolynomialRing
.Fixes #37122.