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

Equality (in the sense of telling identically the same - NOT isomorphic) of toric varieties malfunctions #3934

Closed
HereAround opened this issue Jul 12, 2024 · 4 comments · Fixed by #3946
Labels
bug Something isn't working topic: toric varieties

Comments

@HereAround
Copy link
Member

HereAround commented Jul 12, 2024

The following example is from @lkastner . It produces varieties that are identical, but the check returns false.

julia> p2 = projective_space(NormalToricVariety, 2)
Normal toric variety

julia> fp2 = polyhedral_fan(p2)
Polyhedral fan in ambient dimension 2

julia> sfp2 = star_subdivision(fp2, [1,0])
Polyhedral fan in ambient dimension 2

julia> rays(sfp2) == rays(fp2)
true

julia> maximal_cones(sfp2) == maximal_cones(fp2)
false

julia> n_maximal_cones(sfp2) == n_maximal_cones(fp2)
true

julia> blp2 = normal_toric_variety(sfp2)
Normal toric variety

julia> p2 == blp2
false

I understand that a working equality check is computationally intense.

In the Friday meeting (just briefly discussed the matter there), I heard that irrespective of this performance issue, we should implement the correct equality check. So we should probably try to implement this.

@HereAround HereAround added bug Something isn't working topic: toric varieties labels Jul 12, 2024
@lkastner
Copy link
Member

I want to remark that I am against implementing the equality check you propose. In fact had it been available when we discussed this, you would just have used it and ended up with something much more expensive in #3926. Furthermore I want to remark that there was no consensus in the Friday meeting. Just because the expensive check is implemented for ideals, it does not necessitate to have it for toric varieties.

If you were consistent about this, you would need these checks for all kinds of objects, like schemes, modules, and whatnot. These will be magnitudes more expensive than this check. I have already refactored tons of code that was slow due to comparisons of polyhedra. Comparisons that were unnecessary, since the polyhedra involved were e.g. cones in the same fan or faces of the same polytope, so simple comparisons of index sets would have been enough and much faster. We will end up with tons of devs producing slow code and tons of users complaining about OSCAR being slow.

But since you decided to open this ticket knowing all this I suggest you talk this through with the PIs.

@fieker
Copy link
Contributor

fieker commented Jul 13, 2024 via email

@fieker
Copy link
Contributor

fieker commented Jul 13, 2024 via email

@HereAround
Copy link
Member Author

Can we minimally implement equality as throwing an error? Possibly explaining the issue and pointing to a potential true equality?

On Fri, 12 Jul 2024, 23:28 Lars Kastner, @.> wrote: I want to remark that I am against implementing the equality check you propose. In fact had it been available when we discussed this, you would just have used it and ended up with something much more expensive in #3926 <#3926>. Furthermore I want to remark that there was no consensus in the Friday meeting. Just because the expensive check is implemented for ideals, it does not necessitate to have it for toric varieties. If you were consistent about this, you would need these checks for all kinds of objects, like schemes, modules, and whatnot. These will be magnitudes more expensive than this check. I have already refactored tons of code that was slow due to comparisons of polyhedra. Comparisons that were unnecessary, since the polyhedra involved were e.g. cones in the same fan or faces of the same polytope, so simple comparisons of index sets would have been enough and much faster. We will end up with tons of devs producing slow code and tons of users complaining about OSCAR being slow. But since you decided to open this ticket knowing all this I suggest you talk this through with the PIs. — Reply to this email directly, view it on GitHub <#3934 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV3VFFGA2BA55VVZXMDZMBC6VAVCNFSM6AAAAABKY5L22CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWGM3TMOBWGM . You are receiving this because you are subscribed to this thread.Message ID: @.>

That sounds like a good approach to me. I will draft a corresponding PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working topic: toric varieties
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants