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

Cache canonical_matrix(::MonomialOrdering) #2499

Merged

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Jun 29, 2023

Implements the proposal of #2498 (comment).

The speed change is shown in #2498 (comment).

Feel free to close this and propose an alternative to this @HechtiDerLachs.

Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

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

I'd suggest not to go via attributes. This just entails another dictionary lookup and this is data structure is very low-level, i.e. it is called with high frequency.

src/Rings/orderings.jl Show resolved Hide resolved
src/Rings/orderings.jl Outdated Show resolved Hide resolved
src/Rings/orderings.jl Outdated Show resolved Hide resolved
@HechtiDerLachs
Copy link
Collaborator

HechtiDerLachs commented Jun 29, 2023

And as @fieker said: Avoiding the lookup of a groebner basis in the first place is also possible and should be quicker.

But thanks for the very quick and constructive response!

@fingolfin
Copy link
Member

This just hides over the actual problem, which is that canonical_matrix is being called far too often. See the discussion in #2099

@fingolfin
Copy link
Member

Lest I be misunderstood: I still think this PR is useful and I am fine with merging it, but I removed the Resolves #2498 because I think it would be a bad idea to keep fixing these "leave issues" instead of attacking the root.

src/Rings/orderings.jl Outdated Show resolved Hide resolved
@simonbrandhorst
Copy link
Collaborator

I am glad for anything that helps my computations to actually terminate.

@lgoettgens lgoettgens force-pushed the lg/cache-canonical_matrix branch 2 times, most recently from cfed019 to f5cb257 Compare June 29, 2023 14:49
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #2499 (f5cb257) into master (7b3acca) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2499      +/-   ##
==========================================
- Coverage   72.93%   72.93%   -0.01%     
==========================================
  Files         406      406              
  Lines       53901    53904       +3     
==========================================
+ Hits        39314    39315       +1     
- Misses      14587    14589       +2     
Impacted Files Coverage Δ
src/Rings/orderings.jl 97.31% <75.00%> (-0.25%) ⬇️

... and 5 files with indirect coverage changes

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.

5 participants