Skip to content

Rename is_vertical to passes_transversality_checks and execute related checks#4694

Merged
HereAround merged 9 commits intooscar-system:masterfrom
HereAround:FixVertical
Mar 4, 2025
Merged

Rename is_vertical to passes_transversality_checks and execute related checks#4694
HereAround merged 9 commits intooscar-system:masterfrom
HereAround:FixVertical

Conversation

@HereAround
Copy link
Copy Markdown
Member

No description provided.

Comment thread experimental/FTheoryTools/src/FamilyOfG4Fluxes/properties.jl Outdated
Comment thread experimental/FTheoryTools/src/FamilyOfG4Fluxes/properties.jl Outdated
HereAround and others added 2 commits March 4, 2025 18:51
Co-authored-by: Andrew P Turner <apturner@mac.com>
Co-authored-by: Andrew P Turner <apturner@mac.com>
@apturner apturner marked this pull request as ready for review March 4, 2025 17:54
Copy link
Copy Markdown
Collaborator

@apturner apturner left a comment

Choose a reason for hiding this comment

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

This looks good to me after those 2 changes, thanks @HereAround

Comment thread experimental/FTheoryTools/docs/src/g4.md
@HereAround HereAround changed the title [FTheoryTools] is_vertical -> respects_poincare_symmetry WIP [FTheoryTools] is_vertical -> passes_transversality_checks Mar 4, 2025
@HereAround HereAround merged commit c0fcef9 into oscar-system:master Mar 4, 2025
@HereAround HereAround deleted the FixVertical branch March 4, 2025 21:46
@HereAround HereAround added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Mar 5, 2025
@HereAround HereAround changed the title [FTheoryTools] is_vertical -> passes_transversality_checks Rename is_vertical to passes_transversality_checks and execute related checks. Mar 5, 2025
@HereAround HereAround added the bug Something isn't working label Mar 5, 2025
@HereAround
Copy link
Copy Markdown
Member Author

HereAround commented Mar 7, 2025

@aaruni96 I cannot seem to find the label now, but could this PR please also be backported to 1.3.1?

@lgoettgens
Copy link
Copy Markdown
Member

@aaruni96 I cannot seem to find the label now, but could this PR please also be backported to 1.3.1?

If I see correctly, this PR renames a function. If this wasn't in experimental this would be a breaking change, thus needing a major bump in the version number. IMO this is therefore not a bugfix that should be backported, but of course the last word is to @aaruni96 and @benlorenz

@benlorenz
Copy link
Copy Markdown
Member

benlorenz commented Mar 7, 2025

Renaming for backports seems wrong, but if I see this correctly this was just renamed in the other direction about a month ago (#4562, i.e. between 1.2.2 to 1.3.0), also without any deprecation or alias. Maybe you can explain this a bit?

We could probably add the new name but with an alias or a deprecation ?
(And I don't really know what execute related checks means)

Edit: I now realize the other renaming was slightly different, but nonetheless the is_vertical name existed only for a very short time.

@HereAround
Copy link
Copy Markdown
Member Author

HereAround commented Mar 7, 2025

@benlorenz Yes, let me elaborate.

About maybe 15 years ago, people in the F-theory community were talking about "fluxes being vertical" (whatever a flux is and whatever this may mean - happy to elaborate, but this is beyond the scope of this answer). By now, the phrase for a flux having this very property is instead "flux passes transversality checks". On top of this wording change, today "flux being vertical" means something entirely different than 15 years ago. Sadly, I have mirrored this historic shift of terminology/meaning in OSCAR.

When I brought in is_vertical, I was in some places of OSCAR using it to mean that it did about 15 years ago. However, in other places, is_vertical was used in today's meaning and some sanity checks, that actually should be executed, were not done. This PR does two things:

  1. The first thing that this PR does is to disentangle the terminology. What we currently have as check in OSCAR is only passes_transversality_checks. In particular, no check is_vertical should currently exist in OSCAR.

  2. With this clarification, some default output must always satisfy passes_transversality_checks. Likewise, some sanity checks must include the transversality check. This latter change is what my cryptic message "execute related checks" refers to. (Btw - this change is what caused the extra long tests to fail earlier this week.)

@benlorenz
Copy link
Copy Markdown
Member

This this is in experimental and only exists in 1.3.0 (considering releases), I would be fine with renaming this and adding an alias or deprecation for 1.3.1.

2. Btw - this change is what caused the extra long tests to fail earlier this week.

This means backporting this PR will break these tests on the release branch?

@HereAround
Copy link
Copy Markdown
Member Author

Oh, I did not think of this. Indeed, this would break the extra long tests on the release branch. So presumably not a good idea to backport this change.

@HereAround
Copy link
Copy Markdown
Member Author

HereAround commented Mar 10, 2025

Unless we also backport a fix for the extra long tests, which I am happy to provide. But then, maybe I should just wait for OSCAR 1.4.0?

fieker pushed a commit that referenced this pull request Mar 17, 2025
---------
Co-authored-by: Andrew P Turner <apturner@mac.com>
@lgoettgens lgoettgens changed the title Rename is_vertical to passes_transversality_checks and execute related checks. Rename is_vertical to passes_transversality_checks and execute related checks May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working 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.

5 participants