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

feat: add regular_character #3393

Merged
merged 6 commits into from
Feb 22, 2024
Merged

feat: add regular_character #3393

merged 6 commits into from
Feb 22, 2024

Conversation

thofma
Copy link
Collaborator

@thofma thofma commented Feb 20, 2024

@thofma

This comment was marked as outdated.

@joschmitt
Copy link
Member

This would resolve #3390 assuming we export class_function, right?

@thofma thofma marked this pull request as ready for review February 20, 2024 08:36
@thofma
Copy link
Collaborator Author

thofma commented Feb 20, 2024

Exporting class_function would be more complicated, so let's keep this PR here focused on adding regular_character.

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Merging #3393 (a61221d) into master (a73b844) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3393   +/-   ##
=======================================
  Coverage   82.10%   82.10%           
=======================================
  Files         557      557           
  Lines       73971    73975    +4     
=======================================
+ Hits        60734    60738    +4     
  Misses      13237    13237           
Files Coverage Δ
src/Groups/group_characters.jl 95.34% <100.00%> (+0.02%) ⬆️

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Actually regular_character can assume that the class of the identity element is the first one, thus the function need not do computations with the group.
(I should state this fact in the docstring for conjugacy_classes for character tables.)

What about providing regular_character(tbl::GAPGroupCharacterTable),
and defining regular_character(G::GAPGroup) = regular_character(character_table(G))?

@fingolfin
Copy link
Member

I think what @ThomasBreuer suggests would be the way to go. Of course we can also do that later if you need this in immediately.

@thofma
Copy link
Collaborator Author

thofma commented Feb 20, 2024

Adjusted following your comments. Please have a look again @ThomasBreuer

Copy link
Member

@ThomasBreuer ThomasBreuer 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. Thanks. Just one minor comment on a docstring.

src/Groups/group_characters.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin enabled auto-merge (squash) February 22, 2024 14:37
@fingolfin fingolfin merged commit c0d7816 into master Feb 22, 2024
21 checks passed
@fingolfin fingolfin deleted the th/reg branch February 22, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants