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

Implement equality for toric varieties #4509

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

paemurru
Copy link
Contributor

The only computationally expensive part is computing the rays of the toric varieties, which are usually already computed anyway.

The only computationally complex part is computing the rays of the toric varieties, which are usually already computed anyway.
@joschmitt
Copy link
Member

I vaguely remember that there was a longer discussion about this (parts of it are in #3934, #3946) and eventually it was decided to do things like they are (with the information about slow_equal in the docstring). Did something change?

@HereAround @lkastner

@HereAround
Copy link
Member

I vaguely remember that there was a longer discussion about this (parts of it are in #3934, #3946) and eventually it was decided to do things like they are (with the information about slow_equal in the docstring). Did something change?

@HereAround @lkastner

@paemurru seems to have an increased interest in this feature/functionality. After several discussions more (the last of which I recall did take place before Christmas among @lkastner @paemurru and myself), it was agreed that one could implement such a method - provided interest and time. It seems that @paemurru found both! (Thank you!)

@HereAround
Copy link
Member

@paemurru given the failed required test, I assume this is WIP. Please ping me once ready for review.

@HereAround HereAround added the WIP NOT ready for merging label Jan 28, 2025
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.55%. Comparing base (0d59f06) to head (985293e).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4509   +/-   ##
=======================================
  Coverage   84.55%   84.55%           
=======================================
  Files         672      672           
  Lines       88833    88860   +27     
=======================================
+ Hits        75109    75136   +27     
  Misses      13724    13724           
Files with missing lines Coverage Δ
...oricVarieties/NormalToricVarieties/constructors.jl 100.00% <100.00%> (+3.63%) ⬆️

... and 5 files with indirect coverage changes

@lkastner
Copy link
Member

What is this needed for?

@lkastner lkastner changed the title Implement fast equality for toric varieties Implement equality for toric varieties Jan 28, 2025
@paemurru
Copy link
Contributor Author

@HereAround Those three test failures are from nightly, those are not required or relevant here! This is not a WIP, that tag can be removed

@paemurru
Copy link
Contributor Author

@joschmitt One thing that changed is that I wrote a fast equality check

@lkastner
Copy link
Member

@joschmitt One thing that changed is that I wrote a fast equality check

It is not fast, since it still potentially involves double description computation. This is why I am asking for a use case. Because in the long run we will have users using this check and then complain about Oscar being slow. Most of the time this equality check can be avoided by studying the underlying mathematics. For example we were able to avoid a lot of equality checks of polyhedra in the git fans that way. Often one is in the situation where one fan is already refining the other fan, e.g. for blow ups. Adding this operator makes this check convenient and seemingly cheap. It is not.

@paemurru
Copy link
Contributor Author

@lkastner

It is not fast, since it still potentially involves double description computation.

All the "famous toric varieties" in the documentation already have rays computed, so this does not involve double description computation for them. For any kind of blowups or refining of fans by adding a ray, again the rays are already computed. Often one works with the Cox ring, so the rays need to be computed. I cannot think of a use case where the rays are not already computed.

Would you prefer it if it first checked whether the rays are already computed and if not, it would return an error?

@paemurru
Copy link
Contributor Author

Should be ready to merge now

Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

I cannot say anything about the concrete implementations. But two comments regarding the docstrings below

@benlorenz benlorenz dismissed their stale review January 29, 2025 15:57

resolved

@benlorenz
Copy link
Member

benlorenz commented Jan 29, 2025

Is it clear (and maybe checked?) that the polyhedral fans for these varieties are pointed (lineality_dim == 0)? I could not find anything regarding this in the docs, and otherwise all this breaks down since the rays are not unique.

@paemurru
Copy link
Contributor Author

@benlorenz According to the book of Cox–Little–Schenck, polyhedral fans are pointed. At least according to the documentation, toric geometry in OSCAR follows the book of Cox. I am not sure if this is actually checked in the constructor (it should be!)

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
@joschmitt joschmitt removed the WIP NOT ready for merging label Jan 30, 2025
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.

6 participants