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] Introduce Chern classes of tangent bundle #3996

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

HereAround
Copy link
Member

@HereAround HereAround commented Aug 6, 2024

This PR is inspired by developments in FTheoryTools. It aims to provide functionality to compute the Chern classes of the tangent bundle of a simplicial and complete toric variety.

@HereAround HereAround marked this pull request as draft August 6, 2024 14:47
@HereAround HereAround force-pushed the ChernClassesToricSpace branch 3 times, most recently from 49e4499 to 9b05943 Compare August 6, 2024 15:03
@HereAround
Copy link
Member Author

@lkastner As of right now, I am not happy with the performance of this method.

I have a particular toric variety in mind for which i would like to be able to compute the 4-th Chern class of the tangent bundle. As of right now, this seems almost hopeless with the current implementation. Suggestions/advice appreciated.

Sadly, I cannot attach the corresponding .mrdi file here as github says "We don't support that file type". But I will of course be happy to share said file e.g. via slack with anyone interested.

@HereAround HereAround force-pushed the ChernClassesToricSpace branch 2 times, most recently from 30b5286 to 75eb995 Compare August 9, 2024 15:01
@HereAround HereAround force-pushed the ChernClassesToricSpace branch 2 times, most recently from 7fb8204 to 3ff0665 Compare August 20, 2024 19:18
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 85.36585% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.62%. Comparing base (38cc0a9) to head (3889afd).
Report is 9 commits behind head on master.

Files Patch % Lines
...ies/NormalToricVarieties/standard_constructions.jl 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3996      +/-   ##
==========================================
+ Coverage   84.56%   84.62%   +0.05%     
==========================================
  Files         597      600       +3     
  Lines       82190    82596     +406     
==========================================
+ Hits        69506    69894     +388     
- Misses      12684    12702      +18     
Files Coverage Δ
...ental/IntersectionTheory/src/IntersectionTheory.jl 100.00% <ø> (ø)
...cVarieties/CohomologyClasses/special_attributes.jl 100.00% <100.00%> (ø)
...ies/NormalToricVarieties/standard_constructions.jl 95.10% <0.00%> (-4.17%) ⬇️

... and 28 files with indirect coverage changes

@HereAround HereAround force-pushed the ChernClassesToricSpace branch 4 times, most recently from bc670d9 to b22d58d Compare August 22, 2024 11:33
@HereAround HereAround marked this pull request as ready for review August 22, 2024 13:34
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 have some comments about inferability and thus making the code a bit faster. Please have a look and replace the placeholder types by the actual ones (if possible)

@HereAround
Copy link
Member Author

I have some comments about inferability and thus making the code a bit faster. Please have a look and replace the placeholder types by the actual ones (if possible)

Thank you @lgoettgens . Let me work those changes in.

@HereAround
Copy link
Member Author

I have some comments about inferability and thus making the code a bit faster. Please have a look and replace the placeholder types by the actual ones (if possible)

Thank you @lgoettgens . Let me work those changes in.

Done.


# Examples
```jldoctest
julia> F3 = hirzebruch_surface(NormalToricVariety, 3)
Copy link
Member

Choose a reason for hiding this comment

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

As long as hirzebruch_surface can only return NormalToricVariety, this argument is redundant.

Just in case you were asking: no, it does not make things globally more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have just added simplified constructors in 3889afd. This should (hopefully) fix this issue.

Copy link
Member

Choose a reason for hiding this comment

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

It is not redundant here, as adding a default will keep this default necessary in all of Oscar 1.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I removed it again. To be discussed at a convenient time/setting.

HereAround added a commit to HereAround/Oscar.jl that referenced this pull request Aug 28, 2024
@HereAround HereAround enabled auto-merge (rebase) August 28, 2024 10:37
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.

The changes in 3889afd (#3996) are now in conflict with what was decided upon in #922.

In particular, there should only be a default added for all of these, if this is discussed with people from all areas that already have (or could add) another implementation of this.

@HereAround
Copy link
Member Author

HereAround commented Aug 28, 2024

The changes in 3889afd (#3996) are now in conflict with what was decided upon in #922.

In particular, there should only be a default added for all of these, if this is discussed with people from all areas that already have (or could add) another implementation of this.

Fine @lgoettgens . Removed again and to be discussed at a convenient time/setting then.

@lgoettgens lgoettgens dismissed their stale review August 28, 2024 11:38

The concerning commit has been dropped.

@HereAround HereAround enabled auto-merge (rebase) August 28, 2024 11:39
@HereAround HereAround merged commit 5edd2b0 into oscar-system:master Aug 28, 2024
51 of 53 checks passed
@HereAround HereAround deleted the ChernClassesToricSpace branch August 28, 2024 13:23
@micjoswig
Copy link
Member

The changes in 3889afd (#3996) are now in conflict with what was decided upon in #922.

I do not see this as a conflict, unless someone has a precise plan to implement Chern classes for arbitrary varieties or schemes. The example back then were projective spaces and projective planes which pop up everywhere (and already exits in Oscar).

It is not so easy to derive a design from a few general principles; sometimes it is good to keeping things lean.

@lgoettgens
Copy link
Member

The changes in 3889afd (#3996) are now in conflict with what was decided upon in #922.

I do not see this as a conflict, unless someone has a precise plan to implement Chern classes for arbitrary varieties or schemes. The example back then were projective spaces and projective planes which pop up everywhere (and already exits in Oscar).

It is not so easy to derive a design from a few general principles; sometimes it is good to keeping things lean.

Since the (now again removed) commit doesn't seem to be available on github anymore, let me summarize its changes: it did not only add a shorthand for hirzebruch_surface (which would have been fine for me), but as well for affine_space, projective_space and `weighted_projective space. So it indeed was relevant for the liked issue about projective spaces.

@micjoswig
Copy link
Member

Since the (now again removed) commit doesn't seem to be available on github anymore, let me summarize its changes: it did not only add a shorthand for hirzebruch_surface (which would have been fine for me), but as well for affine_space, projective_space and `weighted_projective space. So it indeed was relevant for the liked issue about projective spaces.

Sorry for creating a mess. My comment only refered to hirzebruch.

@HereAround
Copy link
Member Author

I opened a new PR #4051 regarding the above discussion on hirzebruch_surface(NormalToricVariety, 3) and alike.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants