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

[FTheoryTools] Fixed some bugs that were missed before #4182

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

emikelsons
Copy link
Collaborator

@emikelsons emikelsons commented Oct 7, 2024

Some small bug fixes that were missed in some recents pull requests relating to model indexes and zero section classes of FTheory models:

  • model_index doesn't result in errors anymore(there might have been a defunct method that was used before)
  • Model constructors now no longer attempt to set a zero_section_class for models constructed over an arbitrary base
  • For Tate and Weierstrass models the zero section class is now always set to be 'z'

@emikelsons emikelsons marked this pull request as draft October 7, 2024 14:36
@emikelsons
Copy link
Collaborator Author

@apturner @HereAround

Some small bug fixes that were missed in some recents pull requests relating to model indexes and other stuff.
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.65%. Comparing base (759becf) to head (96b0331).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
.../FTheoryTools/src/LiteratureModels/create_index.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4182      +/-   ##
==========================================
+ Coverage   84.58%   84.65%   +0.06%     
==========================================
  Files         631      631              
  Lines       84829    85271     +442     
==========================================
+ Hits        71756    72184     +428     
- Misses      13073    13087      +14     
Files with missing lines Coverage Δ
.../FTheoryTools/src/AbstractFTheoryModels/methods.jl 78.27% <ø> (ø)
.../FTheoryTools/src/LiteratureModels/constructors.jl 93.85% <100.00%> (+0.04%) ⬆️
...rimental/FTheoryTools/src/TateModels/attributes.jl 86.58% <ø> (ø)
...l/FTheoryTools/src/WeierstrassModels/attributes.jl 86.53% <ø> (ø)
.../FTheoryTools/src/LiteratureModels/create_index.jl 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

@emikelsons emikelsons marked this pull request as ready for review October 9, 2024 14:51
@lgoettgens lgoettgens closed this Oct 10, 2024
@lgoettgens lgoettgens reopened this Oct 10, 2024
@HereAround
Copy link
Member

HereAround commented Oct 11, 2024

Looks good to me. Unless you are still working on this @emikelsons , it can be merged.

Regarding the failed book-tests (cf. https://github.com/oscar-system/Oscar.jl/actions/runs/11269198738/job/31337353429), which prevent the merge at this point: I could not see exactly why they failed. The error message "The operation was cancelled" was not very enlightening to me. So I just restarted those tests.

One possibly very naive idea: The book tests failed after about 2.5 hours. Could it maybe be a time-out error?

@emikelsons
Copy link
Collaborator Author

The error that was raised previously was not a time-out error, but that one doesn't seem to appear any longer

@HereAround
Copy link
Member

The error that was raised previously was not a time-out error, but that one doesn't seem to appear any longer

It seems the error has disappeared. As such, I will merge this now.

Copy link
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 for working on this @emikelsons

@HereAround HereAround merged commit 6e58f05 into oscar-system:master Oct 11, 2024
55 of 56 checks passed
@emikelsons emikelsons deleted the BugFixes branch October 11, 2024 15:00
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.

3 participants