Skip to content

Fix Chevalley basis; add more functionality#4695

Merged
lgoettgens merged 9 commits intooscar-system:masterfrom
lgoettgens:lg/fix-chevalley-basis
Mar 9, 2025
Merged

Fix Chevalley basis; add more functionality#4695
lgoettgens merged 9 commits intooscar-system:masterfrom
lgoettgens:lg/fix-chevalley-basis

Conversation

@lgoettgens
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens commented Mar 4, 2025

Reported by Willem de Graaf on slack. The example was added to the conformance tests, the conformance tests were improved to fail on that example, and then the algorithm was fixed (or rather: implemented properly instead of hoping for the best).

Release notes

  • Fix chevalley_basis (sometimes returning wrong results)
  • Add change_base_ring(::Field, ::LieAlgebra)
  • Add structure_constant_table(::LieAlgebra, basis)

@lgoettgens lgoettgens added topic: lie theory release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes bug: wrong result labels Mar 4, 2025
@lgoettgens lgoettgens requested a review from fingolfin March 4, 2025 17:57
@thofma

This comment was marked as resolved.

@lgoettgens

This comment was marked as resolved.

@thofma

This comment was marked as resolved.

@lgoettgens lgoettgens added backport 1.3.x backport for release branch 1.3 and removed backport 1.3.x backport for release branch 1.3 labels Mar 5, 2025
@lgoettgens
Copy link
Copy Markdown
Member Author

@aaruni96 would this also be able to get backported? If you decide not to due to the amount of new functionality that I needed to add to allow for a proper implementation to fix the bug, that is fine for me as well.

And of course this first needs a review (bump @fingolfin), so please don't wait unnecessarily long with releasing 1.3.1 just because of this. If we do a 1.3.2 sometime, we can then consider this, or it would need to wait for 1.4.0

@aaruni96
Copy link
Copy Markdown
Member

aaruni96 commented Mar 7, 2025

I am vary of adding new functionality to a patch release (if I understand correctly, major version bump = change whatever you want, point version bump = maintain backwards compatibility, patch version bump = bugfixes and very important changes only).

Maybe if we break this PR out into two, one fixing the chevalley_basis, and another for the rest, we could add in the bugfix to 1.3.1 ?

I am of course happy to defer to @benlorenz's judgement.

@lgoettgens
Copy link
Copy Markdown
Member Author

I am vary of adding new functionality to a patch release (if I understand correctly, major version bump = change whatever you want, point version bump = maintain backwards compatibility, patch version bump = bugfixes and very important changes only).

Maybe if we break this PR out into two, one fixing the chevalley_basis, and another for the rest, we could add in the bugfix to 1.3.1 ?

Unfortunately, there is no rest. The new functions are an essential part of the fix, but since they also seem to be useful on their own, I put them into their own functions.

The only split up I could think of is the following:

  1. The fix, the corresponding tests, including all the helper functions (but rename the helper functions by prepending a _)
  2. Remove the added _ from the names, thus making these helper functions "public"

But I am not really sure how useful this even is.

@aaruni96
Copy link
Copy Markdown
Member

aaruni96 commented Mar 7, 2025

If adding functionality is a side effect of a bug fix, then, sure I guess (modulo approval from @fingolfin or so).

@benlorenz
Copy link
Copy Markdown
Member

I think it is fine to add to 1.3.1.

This is also part of experimental so there are less strict rules anyway.

@aaruni96 aaruni96 mentioned this pull request Mar 7, 2025
10 tasks
Comment thread experimental/LieAlgebras/src/LieAlgebra.jl Outdated
Co-authored-by: Max Horn <max@quendi.de>
Comment thread experimental/LieAlgebras/src/LieAlgebra.jl Outdated
@lgoettgens lgoettgens enabled auto-merge (squash) March 9, 2025 08:02
@lgoettgens lgoettgens merged commit 0804f35 into oscar-system:master Mar 9, 2025
aaruni96 pushed a commit that referenced this pull request Mar 10, 2025
(cherry picked from commit 0804f35)
Signed-off-by: Aaruni Kaushik <aaruni@edufor.me>
@lgoettgens lgoettgens deleted the lg/fix-chevalley-basis branch March 10, 2025 19:38
aaruni96 added a commit that referenced this pull request Mar 14, 2025
Manually fix changelog from notes in #4695

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
aaruni96 added a commit that referenced this pull request Mar 14, 2025
* Update changelog on 2025-03-14

* Manually Fix Changelog

Manually fix changelog from notes in #4695

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>

---------

Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>
Co-authored-by: Aaruni Kaushik <aaruni96@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
aaruni96 added a commit that referenced this pull request Mar 14, 2025
* Update changelog on 2025-03-14

* Manually Fix Changelog

Manually fix changelog from notes in #4695

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>

---------

Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>
Co-authored-by: Aaruni Kaushik <aaruni96@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
(cherry picked from commit f2ba7e3)
Signed-off-by: Aaruni Kaushik <aaruni@edufor.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.3.x backport for release branch 1.3 bug: wrong result release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: lie theory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants