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

Update book example for GenericCharacterTables #4163

Merged

Conversation

fingolfin
Copy link
Member

This syncs it with the revised version in the book repo.

I've not tested this locally yet and will fix it up later today. So while I won't complain if someone wants to jump in and help out with any issues in here, please don't feel obliged. Ideally I'll complete this and then afterwards @benlorenz and @lkastner can have a final sanity check / point out anything I also should adjust.

I also plan to update https://github.com/oscar-system/book.oscar-system.org later. Let me know if there are other places that need to be adjusted.

@lgoettgens lgoettgens added the oscar book PRs necessary for the Oscar book label Sep 30, 2024
@fingolfin
Copy link
Member Author

Oops, there is a warning

WARNING: importing deprecated binding AbstractAlgebra.addeq! into GenericCharacterTables.
, use add! instead.

we'll take care of it.

@SoongNoonien can you please remove the addeq! import & replace the addeq! method? Something like this should do: replace

add!(x::GenericCyclo, y::GenericCyclo, z::GenericCyclo) = y+z

function addeq!(x::GenericCyclo, y::GenericCyclo)
	mergewith!(+, x.f, y.f)
	strip_zeros!(x.f)
	return x
end

by

function add!(x::GenericCyclo, y::GenericCyclo, z::GenericCyclo)
        if x !== y
		x.f = deepcopy(y.f)
	end
	mergewith!(+, x.f, z.f)
	strip_zeros!(x.f)
	return x
end

(untested, and I may have missed something else, so please double check it and ideally add a test case?)

@fingolfin
Copy link
Member Author

Apparently there are deeper issues running this against Oscar master (it works against the latest Oscar release). We'll resolve this, too

@fingolfin fingolfin marked this pull request as draft September 30, 2024 09:06
@lgoettgens

This comment was marked as outdated.

@SoongNoonien

This comment was marked as outdated.

@fingolfin fingolfin closed this Oct 1, 2024
@fingolfin fingolfin reopened this Oct 1, 2024
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.68%. Comparing base (4a5b96d) to head (224a818).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4163      +/-   ##
==========================================
- Coverage   84.69%   84.68%   -0.02%     
==========================================
  Files         628      628              
  Lines       84449    84471      +22     
==========================================
+ Hits        71527    71535       +8     
- Misses      12922    12936      +14     

see 5 files with indirect coverage changes

This syncs it with the revised version in the book repo
@fingolfin fingolfin force-pushed the mh/GenericCharacterTables branch from 35e88ce to 224a818 Compare October 1, 2024 07:26
@fingolfin fingolfin marked this pull request as ready for review October 1, 2024 23:14
Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

Looks reasonable and seems to work fine.

@afkafkafk13 afkafkafk13 merged commit c339c2b into oscar-system:master Oct 2, 2024
28 checks passed
@fingolfin fingolfin deleted the mh/GenericCharacterTables branch October 2, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
oscar book PRs necessary for the Oscar book
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants