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] More improvements #4555

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

HereAround
Copy link
Member

@HereAround HereAround commented Feb 6, 2025

  • Avoid "base =" in doctests (code snippet will not execute in repl)
  • Fix the extra-long tests (undefined variable h2)

cc @apturner @emikelsons

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.42%. Comparing base (ca6338c) to head (71b3b06).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4555      +/-   ##
==========================================
- Coverage   84.43%   84.42%   -0.02%     
==========================================
  Files         673      673              
  Lines       89624    89624              
==========================================
- Hits        75676    75662      -14     
- Misses      13948    13962      +14     
Files with missing lines Coverage Δ
...mental/FTheoryTools/src/TateModels/constructors.jl 97.46% <ø> (ø)
...FTheoryTools/src/WeierstrassModels/constructors.jl 95.65% <ø> (ø)

... and 4 files with indirect coverage changes

@HereAround HereAround enabled auto-merge (squash) February 7, 2025 08:03
@emikelsons
Copy link
Collaborator

I see that "base =" also appears in /test/weierstrass_models.jl and /test/tate_models.jl, are these instances a potential issue?

@HereAround
Copy link
Member Author

@benlorenz I have just pushed a change here which should fix the extra-long tests. Please take a look if this PR is ready for merge.

The changes are rather trivial/do not involve an F-theory "magic":

  • First commit: Changed variable name in the doc tests/examples, so that every code snippet (on F-theory) does successfully execute when a user copies it to the REPL. (Currently, two doctests/examples include the assignment base = which errors at least in my Julia REPL).
  • Second commit: Said fix for the extra long tests.

(Alternatively, I can of course make a PR just with the 2nd commit only.)

@lgoettgens lgoettgens added topic: FTheoryTools experimental Only changes experimental parts of the code labels Feb 7, 2025
@HereAround
Copy link
Member Author

HereAround commented Feb 7, 2025

I see that "base =" also appears in /test/weierstrass_models.jl and /test/tate_models.jl, are these instances a potential issue?

While we are at it, probably best to fix them too. I will add this to the next PR though, so that the extra long tests are fixed quickly.

Copy link
Collaborator

@emikelsons emikelsons left a comment

Choose a reason for hiding this comment

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

Looks good and the tests pass

@HereAround HereAround merged commit 1c7f88f into oscar-system:master Feb 7, 2025
26 of 32 checks passed
@HereAround HereAround deleted the MoreImprovements branch February 7, 2025 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code topic: FTheoryTools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants