Skip to content
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

New comparison mode to lazy series and better undefined check #35485

Merged
merged 8 commits into from
Sep 16, 2023

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Apr 12, 2023

📚 Description

This fixes #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 simplify bool and make uninitialized streams nonzero unless they are trivially zero #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

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@tscrim tscrim requested a review from mantepse April 12, 2023 08:35
@tscrim tscrim marked this pull request as draft April 12, 2023 08:36
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 12, 2023

I haven't added all of the requisite doctests, but this is what I arrived at.

This is my proposal for a compromise solution from #35429 with what I want (verification of computations) and what @mantepse wants with speed and usability.

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 12, 2023

One quick comment as I go out the door. In the last commit, I gave another variation on this, where instead of Unknown being returned, we return None for the comparisons and have the bool() return True. This almost passes all doctests except for one that I don't have time to understand, but can likely be easily fixed.

@mantepse
Copy link
Collaborator

Sorry, u probably won't be able to look at this until Saturday.

@tscrim tscrim force-pushed the lazy_series/check_undefined branch from 95bd489 to e81f93d Compare April 13, 2023 00:12
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 13, 2023

No problem; no rush. I fixed the doctest failure. I did a force push of it to keep the alternative proposal within the one commit.

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 18, 2023

The benefits for the last proposal as it (functionally) never gives a false positive for == and != as it returns None in place of Unknown for comparisons it can't know the result. (There are other changes that I would do if we go this route.) However, I think with it not necessarily matching bool(f) and the fact that code needs to be much more careful about f != g versus the less natural not (f == g) (which is typically is not, as evidenced by even the lazy series code needed to be changed), means we should not do this (at least as a default). I could add this as an even further additional option, but perhaps that is diminishing returns with comparison behaviors...

What do you think?

@tscrim tscrim force-pushed the lazy_series/check_undefined branch from e81f93d to fdc7529 Compare June 5, 2023 01:26
mantepse added a commit to mantepse/sage that referenced this pull request Aug 20, 2023
@tscrim tscrim force-pushed the lazy_series/check_undefined branch from 81cb4ec to a3fec59 Compare August 30, 2023 06:17
@tscrim tscrim marked this pull request as ready for review September 7, 2023 15:47
@tscrim
Copy link
Collaborator Author

tscrim commented Sep 7, 2023

This does not need to depend on #35362 as the new streams added have no way of verifying (i.e., an undecidable problem) if they have uninitialized streams being created in the iterator.

Copy link
Collaborator

@mantepse mantepse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of things from #35480 that were lost but should be included:

diff --git a/src/sage/data_structures/stream.py b/src/sage/data_structures/stream.py
index 7038ce2149c..8cd49565fa8 100644
--- a/src/sage/data_structures/stream.py
+++ b/src/sage/data_structures/stream.py
@@ -968,6 +723,11 @@ class Stream_function(Stream_inexact):
     - ``approximate_order`` -- integer; a lower bound for the order
       of the stream
 
+    .. WARNING::
+
+        We assume for equality that ``function`` is a function in the
+        mathematical sense.
+
     EXAMPLES::
 
         sage: from sage.data_structures.stream import Stream_function
@@ -1000,6 +760,43 @@ class Stream_function(Stream_inexact):
         super().__init__(is_sparse, true_order)
         self._approximate_order = approximate_order
 
+    def __hash__(self):
+        """
+        Return the hash of ``self``.
+
+        EXAMPLES::
+
+            sage: from sage.data_structures.stream import Stream_function
+            sage: f = Stream_function(lambda n: n, True, 0)
+            sage: g = Stream_function(lambda n: 1, False, 1)
+            sage: hash(f) == hash(g)
+            True
+        """
+        # We don't hash the function as it might not be hashable.
+        return hash(type(self))
+
+    def __eq__(self, other):
+        """
+        Return whether ``self`` and ``other`` are known to be equal.
+
+        INPUT:
+
+        - ``other`` -- a stream
+
+        EXAMPLES::
+
+            sage: from sage.data_structures.stream import Stream_function
+            sage: fun = lambda n: n
+            sage: f = Stream_function(fun, True, 0)
+            sage: g = Stream_function(fun, False, 0)
+            sage: h = Stream_function(lambda n: n, False, 0)
+            sage: f == g
+            True
+            sage: f == h
+            False
+        """
+        return isinstance(other, type(self)) and self.get_coefficient == other.get_coefficient
+
 
 class Stream_uninitialized(Stream_inexact):
     r"""
@@ -2759,6 +2406,11 @@ class Stream_map_coefficients(Stream_inexact):
     - ``series`` -- a :class:`Stream`
     - ``function`` -- a function that modifies the elements of the stream
 
+    .. WARNING::
+
+        We assume for equality that ``function`` is a function in the
+        mathematical sense.
+
     EXAMPLES::
 
         sage: from sage.data_structures.stream import (Stream_map_coefficients, Stream_function)

src/sage/data_structures/stream.py Outdated Show resolved Hide resolved
src/sage/data_structures/stream.py Outdated Show resolved Hide resolved
src/sage/rings/lazy_series.py Outdated Show resolved Hide resolved
src/sage/rings/lazy_series.py Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator Author

tscrim commented Sep 9, 2023

Thanks. I have added those changes (although I weakened .. WARNING:: to .. NOTE::, but I am not entirely sure how useful it is). I also expanded Stream_unary a bit to allow it to be inherited more to remove some code redundancy.

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 9, 2023

I also have subsequently checked that merging in #35362 doesn't introduce any failures or conflicts.

@mantepse
Copy link
Collaborator

mantepse commented Sep 9, 2023

There are a few trivial doctest failures in recursive_species.py, you'd just have to replace Uninitialized Lazy Laurent Series with Uninitialized Lazy Series (which I like better anyway).

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 10, 2023

You're right that we cannot have any fully correct is_uninitialized() check for streams involving functions (including map_coefficients). At least this gets us most of the way there, and your improvement certainly helps and covers cases I wasn't considering. I have also added a test for that.

For the infinite products/sums, I could make those work purely in terms of modifying #35362.

I am not convinced we should have a check global option. It is too easy for a user to have disabled it and then subsequently pass a series that breaks the assumptions. It would break through the wall separating the series and the streams (as some streams do their own checking). In either case, we would need an example that we cannot find any way to make work -- and really should work -- before I would say we should implement such things (on a separate PR). I think this one here makes a lot of progress as-is.

Copy link
Collaborator

@mantepse mantepse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@mantepse
Copy link
Collaborator

See #35485 (comment) - indeed, I do not want a global option, I want a check keyword for the element constructor..

Using the new is_uninitialized method, I think it will be only rarely necessary to pass check=False.

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 12, 2023

Thank you.

Most likely, but I am hoping that we will never have to use/implement a check ever. We'll let's see what happens when we come across such a bug.

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 12, 2023

Trivial rebase to fix the merge conflict.

@github-actions
Copy link

Documentation preview for this PR (built with commit 1ad7352; changes) is ready! 🎉

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 14, 2023

Fixes #35071.

Once closed, this will also close #35429 and #35480 (our first two attempts).

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2023
…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
@vbraun vbraun merged commit d412568 into sagemath:develop Sep 16, 2023
11 of 12 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 16, 2023
@tscrim tscrim deleted the lazy_series/check_undefined branch September 18, 2023 07:52
@mantepse mantepse mentioned this pull request Sep 19, 2023
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
…ries

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We implement an `integral()` method for lazy Laurent and power series.
If we raise an error if we perform $\int t^{-1} dt$.

We also prove a way to construct a lazy (power) series as the Taylor
series of a function.

<!-- 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. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [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: ...
-->

- sagemath#35485: Uses the `is_uninitialized()` checking.

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36233
Reported by: Travis Scrimshaw
Reviewer(s): Martin Rubey, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
…ries

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We implement an `integral()` method for lazy Laurent and power series.
If we raise an error if we perform $\int t^{-1} dt$.

We also prove a way to construct a lazy (power) series as the Taylor
series of a function.

<!-- 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. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [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: ...
-->

- sagemath#35485: Uses the `is_uninitialized()` checking.

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36233
Reported by: Travis Scrimshaw
Reviewer(s): Martin Rubey, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
…ries

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We implement an `integral()` method for lazy Laurent and power series.
If we raise an error if we perform $\int t^{-1} dt$.

We also prove a way to construct a lazy (power) series as the Taylor
series of a function.

<!-- 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. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [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: ...
-->

- sagemath#35485: Uses the `is_uninitialized()` checking.

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36233
Reported by: Travis Scrimshaw
Reviewer(s): Martin Rubey, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 21, 2024
…ries

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We implement an `integral()` method for lazy Laurent and power series.
If we raise an error if we perform $\int t^{-1} dt$.

We also prove a way to construct a lazy (power) series as the Taylor
series of a function.

<!-- 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. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [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: ...
-->

- sagemath#35485: Uses the `is_uninitialized()` checking.

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36233
Reported by: Travis Scrimshaw
Reviewer(s): Martin Rubey, Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implicit definition of combinatorial species fails
4 participants