-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
simplify bool and make uninitialized streams nonzero unless they are trivially zero #35429
Conversation
I don’t think I agree with making a universal exception for the |
A def define(self, s):
...
if isinstance(coeff_stream, (Stream_zero, Stream_exact)):
self._coeff_stream = coeff_stream
return |
Basically we did something smart already there. That's good. However, it still could end up being zero, and we don't want to give a false positive. |
This is almost ready for review - the modification exhibits a bug in |
Apparently, sage: NCSF = NonCommutativeSymmetricFunctions(QQ)
sage: NCSF in Rings().Commutative()
False
sage: NCSF.is_commutative()
...
AttributeError: 'NonCommutativeSymmetricFunctions_with_category' object has no attribute 'is_commutative' Should I always use |
Please particularly check
is supposed to mean. Maybe we can put something into the testsuites to check that the categories are what they should be? |
Also, should I rebase this on #35338 or the other way round? Merge conflicts are extremely likely. |
#35429 (comment): I think #35429 (comment): If the base ring is the zero ring, then it is only a finite set, otherwise it is infinite. This should be clear. #35429 (comment): Depends on which one you think will be ready/positively-reviewed first. |
As I look at your recent batch of changes, I am becoming fairly convinced you are taking us backwards and not fixing an underlying problem. I don't see how you protect against things being zero but not known to be such: it seems to be pure blind faith. That is, unless the stream is known to be A test I would want to see added is sage: L.<z> = LazyLaurentSeriesRing(GF(2))
sage: M = L(lambda n: 2*n if n < 10 else 0, valuation=0)
sage: bool(M) |
The problem I wanted to fix originally is the doctest in I am not aware of any computation that was possible before and is not possible now. A wrong result that can be obtained with this branch and that was guarded against before, and that is not garbage-in-garbage-out, would, of course be an important argument. There are, however, some computations which are now possible and resulted in errors before. For example: sage: L.<z> = LazyLaurentSeriesRing(QQ)
sage: M.<x> = LazyPowerSeriesRing(L)
sage: f = L(lambda n: 1 if n == 10 else 0, valuation=0)
sage: x*f
Exactly. However, we use some tricks to make sure that at least some common variations of zero are detected.
I am not sure what would be desirable here. In the proposed branch, this should give sage: M = L(lambda n: n, valuation=0)
sage: bool(M) does. I think it is worse to have the latter fail than have a "false" |
You're allowing a lot more things to be considered nonzero that were not before. This is likely to have very subtle bugs by not properly having guards (even infinite loops with expecting a breakout condition) as I expect to be evidenced by the example I gave. Yes, more computations work, but I still believe you are masking an underlying problem with how we are checking stuff. I don't find arguments "it makes this work" very compelling justification. I am fairly certain there are other ways to fix it (and some underlying equality issues). We previously chose to go the route of verified computations. I think this is a very important point. However, for undecidable statements always returning The codecov is really annoying to have included in the diff. It's not useful and I think even wrong (by indrect tests). |
Infinite loops for garbage input can of course happen, I don't have a problem with that. I do have a problem with non-termination if the input is valid, however. I find the idea of having a global switch determining the behaviour in undecidable situations interesting. I can think of the following options. To facilitate illustration, suppose
and that we did not do any computations yet. Then
Then Then Additionally, we have the option of computing up to Is this right? Which combination of options should we offer? In principle, I am willing to implement them all. I think it would be good to make |
Essentially. For The question is |
No, in the
In fact, the documentation of
I would very much appreciate if you could dig out the class that you remember to return |
For symbolics, creating the Actually, that documentation supports that raising an error is an acceptable option. Unfortunately it doesn’t say why it is not letting itself raise an error. |
I must say that this is a rather weak example.
I think it is according to the documentation - although the documentation is unclear about the result of
I suppose because some things (eg., matrix inversion) might not work, but I don't know. |
But that directly contradicts the doc except you have. it cannot prove it, so it should return |
I have now made all the changes, but I cannot get the doctests to work. One failure is the original one from #35071. I would have thought that declaring an uninitialized stream (i.e., The second failure occurs when the coefficient ring is a lazy ring, as follows:
Computations in this ring cannot work, because in
Since this is absolutely speed critical, I am hesitant to wrap this with a |
Of course,
doesn't work either. I am slightly puzzled:
but
(replacing |
Documentation preview for this PR is ready! 🎉 |
It has to do with optimizations that are done in polynomials with the substitutions. I doubt the For the issues with undefined, we might need to force things to be nonzero when built from things containing an undefined series. It will add a bit of complexity to the code with an |
This seems far too complicated to me, and if I understand correctly we don't even have an example of a bad result with the other approach. |
We have complexity in the code for good reasons. We need a way to prove that an answer is unknown, which is why we need both |
You have not demonstrated the need for this extra feature. I am willing to accept the extra complexity if you show me a problem it solves, i.e. bad output (not garbage in garbage out). If it were a few lines, never mind, but this feature adds hundreds of lines - and potentially makes code slower. |
Adding a |
What? All doctests passed with my version of the code, which was shorter by a lot. Please show bad output with respect to that design, not with respect to the current code which I only did because I wanted to find a middle ground. |
Like I said, you need to test against sage: L.<t> = LazyLaurentSeriesRing(QQ)
sage: f = L(lambda x: 0, valuation=0)
sage: f == L.zero() # this is okay because undecidable
False
sage: f != L.zero() # this is definitely not
True Moreover, you could just have every comparison return Don't include/confuse docstrings with lines of code. Your changes mostly removed a lot of documentation, not actual code. PS - IMO the burden of proof is on the author, not the reviewer. Checking the doctests is not sufficient (especially when you change the output and remove tests). I think you would not like (or at least trust) a paper that said "The theorem is true because it worked on all of the examples we tested." |
I don't understand your comment, but that's probably just me. I admit that I am out of energy. I will probably revert to the last working version, because I do not see a way forward. I fundamentally disagree that documentation does not come with a price. Besides, I just realized that the problem is not with uninitialized series (I never quite understood why that should have been the case): sage: L.<x> = LazyPowerSeriesRing(QQ)
sage: f = L(lambda n: n)
sage: g = 1 + x + x^2/2
sage: g(f) |
I don't see why the last line is not OK. For example, this is exactly what happens also in the symbolic ring. Two elements are considered different if we cannot prove that they are the same.
Could you please include an example where #35480 violates this assumption? After all, in if op is op_NE:
return not (self == other) In fact, the assumption implies that |
Frankly, it is a bit horrifying that symbolics supposedly behave like that (from looking at the code, I am not sure they are as it does have a code path that raises an error). How can I know if the result is correct or unknown? It is exactly because of that why my example's output is bad. I would even say it is worse than the symbolics because it isn't tied to My point is that we need to break the aforementioned assumption to have comparison results where we can know that it is (with the amount computed) unknown, where both comparisons return I didn't mean to imply documentation doesn't come with a price, but I measure is very differently than lines-of-code. |
Your characterisation is at best misleading. As you know, it is not possible to decide zero in symbolics (nor with lazy power series), so what remains is a choice of different behaviours. The choice made in The problem with raising an exception is that the code written for other domains does not work anymore, and, apparently, there is very little benefit. I would not be surprised if there were further problems. I would not be surprised either if you could construct an example where this choice indeed gives a bad result which cannot be fixed differently, but that's still better than not being able to carrying out many natural calculations.
I (think I) understand that you think that "fixing" this one way or another is necessary, but I don't see how to do it. I think you are asking for too much here. The starting point of this ticket was to fix #35071. A fix is provided in #35480. If you think you can do substantially better, please at least explain how. |
Look at my example again: It has
One point I will make about
I wouldn't be surprised if this had problems with infinite loops because it assumed a zero series was nonzero and, e.g., tried to invert it. Not to mention that it apparently gives a clearly wrong result with a defined-as-zero series being not equal to the zero series. The fact that you know the result is correct is a huge benefit, no shenanigans. The current code does work correctly (well...should for defined series), but sometimes you just need to have a few more coefficients known.
Can you be specific about which natural calculations you are talking about here? Again, my proposal for something involving undefined series we should default to having its
I am not talking about the result of I gave an example that is clearly
The current version in Sage doesn't need to be fixed at all IMSO. Again, raising errors when trying to do a computation that we don't know how to do is a good thing to do. It means you can trust every computation and result. I will do a PR tomorrow with my idea for |
sage: L.<t> = LazyLaurentSeriesRing(QQ)
sage: f = L(lambda x: 0, valuation=0)
sage: f == L.zero() # this is okay because undecidable
False
sage: f != L.zero() # this is definitely not
True The definition I suggest to use for
I don't understand this. The representations of
No, it does not. It has nothing to do with "defined" series, as my last example shows. The problem is that there is no way of knowing how many terms to compute. Is
I repeat my last example:
As far as I know, throughout sage
There is no way python could find out that
What? Composing a polynomial with a series fails. That's quite a bad bug, in my opinion.
Please do. Here is my wishlist:
And I'd like to add that I'd be quite pissed if you decide to change the code in |
Another thing that might not be doctested currently but should work is accessing an uninitialized series within a To make it a bit clearer what I mean, here is a silly example, which hopefully still illustrates what I mean:
|
Then how do I tell/verify when things are not equal? That is the question I want you to answer. It seems like you are trying to say "you can't."
Again, I said the user-facing representations/data. Yes, the internal stuff is completely different, but that wasn't what I said. IMO, only very experienced users would be able to find out why they are comparing differently even though all of the data they can publicly get is the same.
Right, but the output is still misleading and it is an extra layer of complexity to understand that
No, but that's my point, the output will never suggests that it knows the answer until it actually does.
I am okay with changing the semantics of sage: L.<x> = LazyPowerSeriesRing(QQ)
sage: f = L(lambda n: 0, valuation=0)
sage: 1 / f # infinite loop However, we currently have protection against this (in say, doing a RREF in a matrix) because we can check that
On the contrary, it is certainly not a good idea to enforce
Right., but that is not my question. How do I find that out when two things are not actually equal when Sage says are not? In other words, how can I find differentiate between an unknown-result-computation and not-actually-equal? Again, I think you're trying to say I can't (without unrolling the coefficient check and finding it myself).
In that case, the failure is because of an optimization, not a mathematical statement. The answer is what the error message says: you need to make sure the series is not zero by computing some parts of it. I don't see it as a bad bug, mostly a minor inconvenience except for when something is secretly
I am not going to change the code in Looking at the relaxed
|
I did my proposal at #35485. However, given that we have a long and useful (although perhaps slightly heated) discussion here, I want to make some more comments here regarding what I did. However, I am out of time for today to make a detailed comment, but I will do so tomorrow. Sorry! |
Okay, I am finally explaining a bit longer about #35485 and what I learned from doing that. As I mentioned, my first implementation there adds an option to how we handle infinite halting precision. Now we can chose between verifiable computations versus potentially never errorring out but false positive My second commit implemented the other semantic that I proposed. If a comparison is not known, then it returns As mentioned on #35485, we can also add an additional global option to have all 3 of these infinite halting precision comparison modes. (It would be a nightmare to make different parents for them.) I am not sure how useful it would be. I will again note that lazy Unfortunately adopting any of those approaches means we won't be able to simplify the code, but I think there is enough utility to offset the (potential) maintenance and complexity. (In particular, the code and doc is already there.) |
…ined check <!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to multiply two integers" --> ### 📚 Description <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This fixes sagemath#35071 by: 1. providing a method to streams to see if they are unitinalized 2. includes a new mode for comparisons in the lazy series ring based on the proposal in sagemath#35429. The new comparison mode `secure` simply returns `False` when it cannot verify that `f == g` and makes sure that `(f == g) == not (f != g)`, and this will become the new default. In particular, it will return `True` for `f != g` both when it cannot show that `f == g` and when they are genuinely different. In order to verify when the comparison is unknown, we expose the `is_nonzero()` that only returns `True` when the series is _known_ to be nonzero. Thus, we verify by `(f - g).is_nonzero()`. When a finite halting precision is given, then that takes priority. For the infinite halting precision in the "old" version (`secure = True`), it will raise a `ValueError` when it cannot verify the result. **NOTE:** `f.is_zero()` is still the default `not f` for speed and the assumption these are in agreement elsewhere in Sage. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35485 Reported by: Travis Scrimshaw Reviewer(s): Martin Rubey, Travis Scrimshaw
…ined check <!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to multiply two integers" --> ### 📚 Description <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This fixes sagemath#35071 by: 1. providing a method to streams to see if they are unitinalized 2. includes a new mode for comparisons in the lazy series ring based on the proposal in sagemath#35429. The new comparison mode `secure` simply returns `False` when it cannot verify that `f == g` and makes sure that `(f == g) == not (f != g)`, and this will become the new default. In particular, it will return `True` for `f != g` both when it cannot show that `f == g` and when they are genuinely different. In order to verify when the comparison is unknown, we expose the `is_nonzero()` that only returns `True` when the series is _known_ to be nonzero. Thus, we verify by `(f - g).is_nonzero()`. When a finite halting precision is given, then that takes priority. For the infinite halting precision in the "old" version (`secure = True`), it will raise a `ValueError` when it cannot verify the result. **NOTE:** `f.is_zero()` is still the default `not f` for speed and the assumption these are in agreement elsewhere in Sage. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35485 Reported by: Travis Scrimshaw Reviewer(s): Martin Rubey, Travis Scrimshaw
See #35485 |
bool
should returnTrue
if we know that the stream is non-zero, andFalse
otherwise. The behaviour of equality should be similar. This simplifies things enormously.📚 Description
Fixes #35071
📝 Checklist
⌛ Dependencies