-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Implicit function solver for lazy series #38108
base: develop
Are you sure you want to change the base?
Implicit function solver for lazy series #38108
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
In an effort to make sagemath#38108 more usable, we optimize the computation of gcd's in generic polynomial rings. URL: sagemath#38924 Reported by: Martin Rubey Reviewer(s): Martin Rubey, mathzeta, Travis Scrimshaw
In an effort to make sagemath#38108 more usable, we optimize the computation of gcd's in generic polynomial rings. URL: sagemath#38924 Reported by: Martin Rubey Reviewer(s): Martin Rubey, mathzeta, Travis Scrimshaw
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. There just a few more details I have seen when giving this another look over.
I have one additional question. This gets rid of the old implicit definition, correct? I expect this to be slower than the "solver" we have before for f = G(f)
. Thus, I am thinking we should keep that implementation for that special case. It's possible I missed something when looking at the code.
src/sage/rings/lazy_series_ring.py
Outdated
|
||
If ``self`` is a univariate Laurent, power, or Dirichlet | ||
series, this is the list containing the one of the base ring. | ||
|
||
If ``self`` is a multivariate power series, this is the list | ||
of monomials of total degree ``n``. | ||
|
||
If ``self`` is a lazy symmetric function, this is the list | ||
of basis elements of total degree ``n``. |
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 data/explanation is better with the concrete implementations as this could fall out of date as new implementations are added.
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.
The specifications are also in the concrete implementations. I slightly reworded to make it clear that this is intended as an explanation of the purpose of the (abstract) method.
src/sage/data_structures/stream.py
Outdated
- ``var``, a variable in ``self._P`` | ||
- ``val``, the value that should replace the variable |
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.
- ``var``, a variable in ``self._P`` | |
- ``val``, the value that should replace the variable | |
- ``var`` -- a variable in ``self._P`` | |
- ``val`` -- the value that should replace the variable |
No, the special case is treated as before - although it is also handled by Yes, |
Ah, okay, thank you. Then once the other details are addressed, then this will be a positive review. |
Cool, I'm on it. Good evening? :-) |
Still the afternoon, but getting late. Greetings from Singapore. I hope you are feeling better now. |
thank you, well, better indeed, but still somewhat so-so :-) Currently running tests. Short summary: |
I cannot reproduce the failure in |
I marked the one doctest failure on macos as random. Apparently, the order of the variables is different on macos than on linux. However, it is more valuable to have the full output of the doctest, and, since it is an error message, I do not want to try to make the error message less random, which would involve some work and introduce more brittleness. |
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 for the changes. This now LGTM. Thank you for all of your hard work in getting this implemented. This is a major new feature.
One last thing before a positive review.
After #39523, the doctests after |
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. Let's get this in. Any additional improvements or fixes can be done on later on followup PRs.
This pull request provides a method `define_implicitly` for lazy rings that allows to produce lazy power series solutions for systems of functional equations, in relatively great generality, superseding sagemath#37033. We use a special action to make sure that coercion between `CoefficientRing` and its base ring works as desired. However, it is a local change, if the current approach eventually shows its shortcomings. (Alternative approach previously considered: Using a special functor to control the pushout behaviors. The action approach was chosen as a way to prevent potential conflicts with other construction functors that may appear in the future.) Dependencies: sagemath#38729 URL: sagemath#38108 Reported by: Martin Rubey Reviewer(s): Martin Rubey, Travis Scrimshaw
This pull request provides a method
define_implicitly
for lazy rings that allows to produce lazy power series solutions for systems of functional equations, in relatively great generality, superseding #37033.We use a special action to make sure that coercion between
CoefficientRing
and its base ring works as desired. However, it is a local change, if the current approach eventually shows its shortcomings. (Alternative approach previously considered: Using a special functor to control the pushout behaviors. The action approach was chosen as a way to prevent potential conflicts with other construction functors that may appear in the future.)Dependencies: #38729