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

do not duplicate cache in Stream_uninitialized #34661

Open
mantepse opened this issue Oct 14, 2022 · 22 comments · May be fixed by #35330
Open

do not duplicate cache in Stream_uninitialized #34661

mantepse opened this issue Oct 14, 2022 · 22 comments · May be fixed by #35330

Comments

@mantepse
Copy link
Collaborator

Currently, an instance of Stream_initialized duplicates the cache of its defining stream.

This ticket aims at removing this redundancy, at least in the case when the defining stream is dense.

Depends on #34636

CC: @tscrim

Component: combinatorics

Keywords: LazyPowerSeries

Author: Martin Rubey

Branch/Commit: u/mantepse/do_not_duplicate_cache_in_stream_uninitialized @ d64d3cb

Issue created by migration from https://trac.sagemath.org/ticket/34661

@mantepse mantepse added this to the sage-9.8 milestone Oct 14, 2022
@mantepse
Copy link
Collaborator Author

@mantepse
Copy link
Collaborator Author

Author: Martin Rubey

@mantepse
Copy link
Collaborator Author

Last 10 new commits:

bd3e3ebmake sparsity a decision of the user
814aa7cremove Stream_cauchy_invert.get_coefficient, make sparse a mandatory argument, move _is_sparse an attribute of Stream_inexact
7043d1cremove redundant `__init__` methods, remove finished TODOs
e6c4caemake Stream_uninitialized always dense to avoid maximal recursion error
9577386implement Polynomial_generic_sparse.__floordiv__
ec1eebdMerge branch 'u/mantepse/__floordiv___for_sparse_polynomials' of trac.sagemath.org:sage into t/34636/make_sparsity_a_decision_of_the_user
de424bdmake internal rings sparse or dense if the lazy series ring is sparse or dense
66c1208Merge branch 'u/mantepse/make_sparsity_a_decision_of_the_user' of trac.sagemath.org:sage into t/34611/fast_implementation_of_exp
5020b9dadapt exp and log to new sparsity
8396a91remove _target

@mantepse

This comment has been minimized.

@mantepse
Copy link
Collaborator Author

Commit: 8396a91

@mantepse
Copy link
Collaborator Author

comment:3

In principle, we could also make Stream_uninitialized inherit sparsity from it's defining stream, I doubt that this makes a big difference.

@mantepse
Copy link
Collaborator Author

comment:4

Unfortunately, the current branch fails. Here is a simplified illustration of what goes wrong:

sage: L.<z> = LazyPowerSeriesRing(ZZ)
sage: A = L.undefined(valuation=5)
sage: B = L.undefined()
sage: A.define(z^5 + B)
sage: B.define(z + A)
sage: A
z^5 + z^9 + z^10 + O(z^12)
sage: B
1 + z^4 + O(z^7)

I think we are sloppy about our use of _approximate_order and _offset, which are now both lazy attributes. This might eventually become a problem anyway (eg. with #34556), so we better solve it:

sage: A = L.undefined(valuation=5); B = L.undefined(); A.define(z^5 + B); B.define(z + A)
sage: A._coeff_stream._offset
5
sage: B._coeff_stream._offset
0
sage: B[0]
1

The iterator of B (which is just the default iterator of Stream_inexact) is

    def iterate_coefficients(self):
        n = self._approximate_order
        while True:
            yield self.get_coefficient(n)
            n += 1

When it is called by B._coeff_stream.__getitem__[0], we find n == self._approximate_order == 1, which does not correspond to self._offset, which is 0.

@mantepse
Copy link
Collaborator Author

Changed keywords from none to LazyPowerSeries

@mantepse
Copy link
Collaborator Author

comment:6

I think the problem is that the _approximate_order in the iterator comes from (z + A)._coeff_stream, i.e., from Stream_add, whereas the _approximate_order used to compute B._coeff_stream._offset comes from B, i.e., Stream_uninitialized.

This is quite confusing :-)

@mantepse
Copy link
Collaborator Author

comment:7

I am guessing, the essence of the problem is that we do not clearly specify with which coefficient Stream.iterate_coefficients begins.

The current behaviour is that it starts with the _approximate_order which is currently valid :-)

@mantepse
Copy link
Collaborator Author

comment:8

I should add that the definition above, A.define(z^5 + B); B.define(z + A), does not make sense with A = L.undefined(valuation=5). That is, it should fail because the valuation of B would then be 1, contradicting the stated valuation of A.

@mantepse
Copy link
Collaborator Author

comment:9

Actually, now I found a bug (or garbage in - garbage out without warning), without this ticket:

sage: L.<z> = LazyPowerSeriesRing(ZZ, sparse=False); A = L.undefined(valuation=3); B = L.undefined(); A.define(z^3 + B); B.define(z^2 + A^2)
sage: A
z^3 + z^6 + 2*z^9 + O(z^10)
sage: B
z^2 + z^6 + O(z^7)

This is wrong, because A != z^3 + B.

@mantepse
Copy link
Collaborator Author

Dependencies: #34636

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

72f2037Merge branch 'develop' of trac.sagemath.org:sage into t/34661/do_not_duplicate_cache_in_stream_uninitialized
d64d3cbexperimentally also redefine the approximate order of the target

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2022

Changed commit from 8396a91 to d64d3cb

@mantepse
Copy link
Collaborator Author

comment:12

Resetting the approximate order of self and of coeff_stream in Stream_uninitialized.define seems to work roughly (unless coeff_stream._is_sparse, of course), but I am quite sure it is not the right way to do it. In particular, there are some failures which I don't quite understand (among others).

My hope would be to have a cleaner design, and more precise responsibilities for approximate_order.

Help would be very much appreciated.

I guess I should go through a simple recursive definition with pen and paper.

@mantepse
Copy link
Collaborator Author

comment:13

@tscrim, in case you have some time, this is the ticket I currently find most important.

I'd like to stress, however, that I do not find the aim as stated in the description important (in most cases, the savings will be negligible). Instead, what I hope for is a much cleaner (and therefore more robust) design.

In particular, this ticket should probably be considered together with #34556.

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2022

comment:14

I am not sure this is possible when the undefined stream has an AO (= Approximate Order) larger than that of the target stream. If undef.AO > target.AO, then when you copy the cache of the target, then you get a misalignment of the undef cache with the AO. In particular, the is_nonzero is assuming that any nonzero entry in the cache corresponds to an entry at least the stream's AO.

@mantepse
Copy link
Collaborator Author

comment:15

Maybe we could have a meeting some time to talk about this. Things are not clear to me at all.

@tscrim
Copy link
Collaborator

tscrim commented Oct 29, 2022

comment:16

That would be good. Send me an email and we can set it up. I am basically free anytime you would be awake (including tomorrow) except for my Wednesday evenings.

@mantepse
Copy link
Collaborator Author

comment:17

I'm afraid I won't make it this week, I'll send mail as soon as I have time!

@tscrim
Copy link
Collaborator

tscrim commented Nov 1, 2022

comment:18

No problem. I will be at a conference next week, but I can talk during my evenings.

@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
@mantepse mantepse linked a pull request Mar 22, 2023 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants