Skip to content

Add exceptional classes and indices#4779

Merged
HereAround merged 3 commits intooscar-system:masterfrom
emikelsons:ExceptionalClasses
Apr 10, 2025
Merged

Add exceptional classes and indices#4779
HereAround merged 3 commits intooscar-system:masterfrom
emikelsons:ExceptionalClasses

Conversation

@emikelsons
Copy link
Copy Markdown
Collaborator

Added the attributes exceptional_classes and exceptional_divisor_indices for FTheory models for more convenient access to this information. I also made slight improvements to the internal _set_all_attributes method.
Before this is merged, I would also like to add changes, that would change the exceptional_classes attribute, whenever a blow up is performed. @HereAround @apturner

@emikelsons emikelsons added topic: FTheoryTools release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Apr 3, 2025
@HereAround HereAround added the WIP NOT ready for merging label Apr 3, 2025
@emikelsons emikelsons force-pushed the ExceptionalClasses branch from cbc844b to 598eb1e Compare April 3, 2025 12:32
@emikelsons emikelsons removed the WIP NOT ready for merging label Apr 4, 2025
@emikelsons emikelsons force-pushed the ExceptionalClasses branch from 9949a5a to ad711ff Compare April 4, 2025 10:15
@HereAround
Copy link
Copy Markdown
Member

@apturner if the tests are correct, these changes here somehow break what we stated in our book chapter. But I fail to see exactly what. Any idea?

(@emikelsons Did you already rebase to master. Just want to make sure that the Hecke documentation error is gone and not responsible for any of this.)

Generally speaking, this looks good to me.

@emikelsons
Copy link
Copy Markdown
Collaborator Author

@apturner if the tests are correct, these changes here somehow break what we stated in our book chapter. But I fail to see exactly what. Any idea?

(@emikelsons Did you already rebase to master. Just want to make sure that the Hecke documentation error is gone and not responsible for any of this.)

Generally speaking, this looks good to me.

I did rebase, but if the errors that persist were indeed caused by me - then I'm not sure. I added additional optional arguments to the blow_up method, but I'm not sure, if this could cause the book tests to fail

@benlorenz
Copy link
Copy Markdown
Member

Regarding the book tests, one ambient space prints slightly different now:

  julia> t1 = blow_up(t, [\"x\", \"y\", \"x1\"]; coordinate_name = \"e1\")
  Partially resolved global Tate model over a concrete base
  
  julia> amb1 = ambient_space(t1)
- Normal toric variety
+ Normal, simplicial toric variety

(https://github.com/oscar-system/Oscar.jl/actions/runs/14264819453/job/39984171773?pr=4779#step:7:921)

Regarding the other tests: This looks like the timeout / IO issues that we have seen quite a bit during this week, this hopefully disappears when re-run when the CI runner is less busy.

@emikelsons emikelsons force-pushed the ExceptionalClasses branch 2 times, most recently from c27ff54 to 3c96c76 Compare April 6, 2025 13:48
@emikelsons emikelsons force-pushed the ExceptionalClasses branch from 3c96c76 to 38a5adf Compare April 7, 2025 07:49
@benlorenz
Copy link
Copy Markdown
Member

It would also be fine to adjust the book-test output to new printing, we never claimed that this would stay unchanged. (assuming the new output is also correct)
Only the input lines should stay unchanged in 1.x

@emikelsons
Copy link
Copy Markdown
Collaborator Author

It would also be fine to adjust the book-test output to new printing, we never claimed that this would stay unchanged. (assuming the new output is also correct) Only the input lines should stay unchanged in 1.x

I see, I wasn't familiar with the book test claims and restrictions. This certainly makes things easier as my current fix isn't great

@emikelsons emikelsons force-pushed the ExceptionalClasses branch from 38a5adf to 7bdb13e Compare April 7, 2025 08:40
Copy link
Copy Markdown
Member

@HereAround HereAround left a comment

Choose a reason for hiding this comment

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

Thank you @emikelsons for working on this!

@HereAround HereAround merged commit 880fb10 into oscar-system:master Apr 10, 2025
32 checks passed
fieker pushed a commit that referenced this pull request May 16, 2025
@emikelsons emikelsons deleted the ExceptionalClasses branch August 15, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: FTheoryTools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants