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

[ToricVarieties] Shorthands for hirzebruch surface and del Pezzo surface #4051

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HereAround
Copy link
Member

@HereAround HereAround commented Aug 28, 2024

As pointed out by @micjoswig in #3996, for the current constructor hirzebruch_surface(::Type{NormalToricVariety}, r::Int), the first argument is currently obsolete. This PR introduces a shorthand hirzebruch_surface(r::Int).

A similar situation exists for del_pezzo_surface, and I have taken the liberty to propose a similar short hand.

We also have the following specialized constructors:

  • affine_space(::Type{NormalToricVariety}, var_names::AbstractVector{<:VarName})
  • affine_space(::Type{NormalToricVariety}, d::Int)
  • projective_space(::Type{NormalToricVariety}, d::Int)
  • weighted_projective_space(::Type{NormalToricVariety}, w::Vector{T}) where {T <: IntegerUnion}

I could not find instances, where those constructors are called with a first argument different from NormalToricVariety. (But of course, I could have overlooked those instances...) In #3996, I was therefore carried away to propose short hands for all of these constructors. This is (at least not yet) done in this PR, as this seems to require discussion and so I removed this proposed change after being alerted to this by @lgoettgens .

This PR aims to clarify the situation and to resolve it.

@HereAround HereAround marked this pull request as draft August 28, 2024 14:07
@HereAround HereAround added topic: toric varieties optimization Simpler/more performant code or more/better tests labels Aug 28, 2024
@lgoettgens
Copy link
Member

Just to note: The current format was decided on in #922.

The other area in Oscar that came to my mind with these function names was geometry/schemes. Thus pinging some people to get opinions on this @jankoboehm @afkafkafk13 @HechtiDerLachs

@afkafkafk13
Copy link
Collaborator

We should postpone the discussion of this point, until @simonbrandhorst is back from his holidays (1.9.).

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.65%. Comparing base (5edd2b0) to head (4970832).

Files with missing lines Patch % Lines
...ies/NormalToricVarieties/standard_constructions.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4051      +/-   ##
==========================================
+ Coverage   84.62%   84.65%   +0.03%     
==========================================
  Files         600      600              
  Lines       82590    82592       +2     
==========================================
+ Hits        69894    69921      +27     
+ Misses      12696    12671      -25     
Files with missing lines Coverage Δ
...ies/NormalToricVarieties/standard_constructions.jl 97.84% <0.00%> (-1.43%) ⬇️

... and 2 files with indirect coverage changes

@simonbrandhorst
Copy link
Collaborator

Agreed, we should add constructors for

  • affine_space(::Type{AffineScheme}, var_names::AbstractVector{<:VarName})
  • affine_space(::Type{AffineScheme}, d::Int)
  • projective_space(::Type{ProjectiveScheme}, d::Int)

So far the shorthands for affine_space etc. just return the above.
One could think about disabling the current shortcuts (would break the interface) or introduce other default
... for me getting an affine space as a scheme always seemed a reasonable default. But of course I am biased here :-).

@afkafkafk13
Copy link
Collaborator

Agreed, we should add constructors for

* `affine_space(::Type{AffineScheme}, var_names::AbstractVector{<:VarName})`

* `affine_space(::Type{AffineScheme}, d::Int)`

* `projective_space(::Type{ProjectiveScheme}, d::Int)`

So far the shorthands for affine_space etc. just return the above. One could think about disabling the current shortcuts (would break the interface) or introduce other default ... for me getting an affine space as a scheme always seemed a reasonable default. But of course I am biased here :-).

Given the existence of different perspectives on such objects in different parts of Oscar and its community, we should probably deprecate the current shorthands and not introduce shorthands for Hirzebruch and DelPezzo surfaces. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Simpler/more performant code or more/better tests topic: toric varieties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants