Skip to content

Reduce yet more randomness in FTheoryTools#5295

Merged
HereAround merged 5 commits intooscar-system:masterfrom
HereAround:FixMoreRandomness
Sep 12, 2025
Merged

Reduce yet more randomness in FTheoryTools#5295
HereAround merged 5 commits intooscar-system:masterfrom
HereAround:FixMoreRandomness

Conversation

@HereAround
Copy link
Copy Markdown
Member

  • Check doc strings - is rng documented (in a uniform way) for all relevant functions?
  • Should the signature of g4_flux_family(gf::G4Flux; completeness_check::Bool = true) be changed? I suppose it should allow to switch the algorithm used to create the g4 flux family by? If so, likely (?) also needs an rng kwarg.
  • Other instances similar to the above?

cc @emikelsons @apturner

@HereAround HereAround added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes extra-long Also run the extra-long tests during CI. topic: FTheoryTools optimization Simpler/more performant code or more/better tests labels Sep 10, 2025
@HereAround HereAround closed this Sep 10, 2025
@HereAround HereAround reopened this Sep 10, 2025
@HereAround HereAround changed the title Fix more randomness in FTheoryTools Reduce yet more randomness in FTheoryTools Sep 10, 2025
@HereAround HereAround marked this pull request as ready for review September 10, 2025 18:13
Copy link
Copy Markdown
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!

@emikelsons
Copy link
Copy Markdown
Collaborator

emikelsons commented Sep 10, 2025

I didn't notice the comment at first, but it makes sense that g4_flux_family should have an optional argument to specify the algorithm

@HereAround
Copy link
Copy Markdown
Member Author

I didn't notice the comment at first, but it makes sense that g4_flux_family should have an optional argument to specify the algorithm

Alright. I will come back to those issues/open ends in the next PR.

@HereAround HereAround merged commit b303396 into oscar-system:master Sep 12, 2025
29 of 31 checks passed
@HereAround HereAround deleted the FixMoreRandomness branch September 12, 2025 10:22
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Nov 26, 2025
@fingolfin
Copy link
Copy Markdown
Member

I've now changed this to use the release notes: use title label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extra-long Also run the extra-long tests during CI. optimization Simpler/more performant code or more/better tests 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