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

Add dev docs on caching parent objects #3619

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 17, 2024

Following the discussions last Friday and again this Wednesday, where Claus explained the caching rules as they were so far; I've now tried to codify them here, for future reference or revision. In particular, what I describe here is not meant to be something new, but reflect what was the policy so far.

I am sure I forgot stuff or maybe even misrepresented something, so please let the feedback flow in.

Resolves #3616

@fingolfin
Copy link
Member Author

I just discovered some notes on caching at https://nemocas.github.io/AbstractAlgebra.jl/dev/ring_interface/#Parent-object-caches -- perhaps some of that should also be included or referenced here (and/or perhaps the text there should be updated). I'll wait to hear what @fieker thinks.

Comment on lines 41 to 43
For interactive use, it is often simply convenient: e.g. taking the characteristic
polynomials of two matrices and multiplying them, without worrying about specifying
the parent rings of the polynomials.
Copy link
Member

Choose a reason for hiding this comment

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

Funnily enough, exactly this example doesn't work because caching is deliberately turned off:

julia> M = QQ[1 0; 0 1];

julia> charpoly(M)*charpoly(M)
ERROR: Incompatible polynomial rings in polynomial operation
[...]

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch! Thank you very much for fact checking this.

That makes me question the point of entire caching thing even more sigh.

So, does anyone then have a not completely unconvincing example for a situation where a cached parent helps a "naive" user?

Comment on lines +45 to +56
Caching parents also has downsides. E.g. all those cached objects take up memory which
in some cases can add up to significant amounts.
Copy link
Member

@joschmitt joschmitt Apr 17, 2024

Choose a reason for hiding this comment

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

Tommy also mentioned issues with parallelization/thread safety as a (possible) downside to me.

(And I don't really understand how the memory could fill up if we use these WeakValueDicts? But that's more for my curiosity.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not all our caches actually use WeakValueDict. We introduced it much later and e.g. Hecke still uses Dict and IdDict caches quite a bit (I just grepped for get_cached! to find examples for caches and then checked their definitions). Might be a good idea for somebody to clean that up.

Copy link
Member

Choose a reason for hiding this comment

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

I hope that I got all caching dictionaries which weren't WeakValueDicts in Nemocas/Nemo.jl#1724 and thofma/Hecke.jl#1460.

Copy link
Member

@joschmitt joschmitt left a comment

Choose a reason for hiding this comment

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

This matches what I was told over the past years. It's good to have a 'reference' now.

@fieker
Copy link
Contributor

fieker commented Apr 18, 2024

Maybe add Hecke.clear_cache() which tries its best to empty all(?) caches in AA, Nemo and Hecke?

@thofma
Copy link
Collaborator

thofma commented Apr 19, 2024

julia> Zx, x = ZZ["x"]
(Univariate polynomial ring in x over ZZ, x)

julia> F = GF(2);

julia> map_coefficients(F, x^2) + map_coefficients(F, x)
x^2 + x

Copy link
Contributor

@fieker fieker left a comment

Choose a reason for hiding this comment

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

it moves into the right direction.

Following the discussions last Friday and again this Wednesday, where
Claus explained the caching rules as they were so far; I've now tried
to codify them here, for future reference or revision. In particular,
what I describe here is not meant to be something new, but reflect
what was the policy *so far*.

[skip ci]

Co-authored-by: Johannes Schmitt <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Tommy Hofmann <[email protected]>
@fingolfin
Copy link
Member Author

Thanks @thofma I've added your nice example.

BTW I find it quite disheartening that I get this on my M1 MacBook the first time I execute map_entries (that's not direct against Tommy or against anyone, really, just something that makes me sad / where we badly need to improve, somehow)

julia> Zx, x = ZZ["x"]
(Univariate polynomial ring in x over ZZ, x)

julia> F = GF(2);

julia> @time map_coefficients(F, x^2);
 11.979656 seconds (60.42 M allocations: 3.764 GiB, 7.85% gc time, 100.00% compilation time)

julia> @time map_coefficients(F, x^2);
  0.000021 seconds (18 allocations: 976 bytes)

@fingolfin fingolfin merged commit 389fb3d into oscar-system:master Apr 22, 2024
@fingolfin fingolfin deleted the mh/caching-docs branch April 22, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a "caching policy" section to OSCAR dev docs
5 participants