Skip to content

Add serialization of FinGenAbGroupHom#5196

Merged
HereAround merged 1 commit intooscar-system:masterfrom
HereAround:MakeClausHappy-1
Aug 15, 2025
Merged

Add serialization of FinGenAbGroupHom#5196
HereAround merged 1 commit intooscar-system:masterfrom
HereAround:MakeClausHappy-1

Conversation

@HereAround
Copy link
Copy Markdown
Member

@HereAround HereAround commented Aug 15, 2025

cc @antonydellavecchia: Splitting-off changes in #5064 as discussed.

@HereAround HereAround force-pushed the MakeClausHappy-1 branch 6 times, most recently from e1c263f to 0b277bd Compare August 15, 2025 09:34
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

this should also get some pure FinGenAbGroupHom serialization tests (so without any toric stuff in the test). IMO it would even make sense for the PR to be split up into two parts: 1. just adding serialization of FinGenAbGroupHom, 2. using the first PR in a toric setting
That way we could have better release notes for the first part

Comment thread src/Serialization/Groups.jl Outdated
@HereAround HereAround added extra-long Also run the extra-long tests during CI. enhancement New feature or request serialization release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Aug 15, 2025
@HereAround HereAround closed this Aug 15, 2025
@HereAround HereAround reopened this Aug 15, 2025
@HereAround HereAround force-pushed the MakeClausHappy-1 branch 2 times, most recently from 93c8110 to fe0bbe5 Compare August 15, 2025 10:22
@HereAround HereAround changed the title [ToricVarieties] SerializeMap from Weil divisors to Class group Serialize FinGenAbGroupHom Aug 15, 2025
@HereAround HereAround added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: groups and removed 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. labels Aug 15, 2025
@HereAround HereAround marked this pull request as ready for review August 15, 2025 10:28
@HereAround HereAround requested a review from lgoettgens August 15, 2025 10:29
@HereAround
Copy link
Copy Markdown
Member Author

this should also get some pure FinGenAbGroupHom serialization tests (so without any toric stuff in the test). IMO it would even make sense for the PR to be split up into two parts: 1. just adding serialization of FinGenAbGroupHom, 2. using the first PR in a toric setting That way we could have better release notes for the first part

Hopefully closer to your desire now, or so I hope. Please take another look.

Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

yeah, I should have given you some more than 3 minutes after opening a PR to bring it in good shape :)

@HereAround HereAround enabled auto-merge (rebase) August 15, 2025 10:34
@lgoettgens lgoettgens disabled auto-merge August 15, 2025 12:30
@HereAround
Copy link
Copy Markdown
Member Author

Failures in this PR likely due to #5201.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.84%. Comparing base (304c6be) to head (3e40fef).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5196   +/-   ##
=======================================
  Coverage   84.83%   84.84%           
=======================================
  Files         710      710           
  Lines       95508    95514    +6     
=======================================
+ Hits        81025    81038   +13     
+ Misses      14483    14476    -7     
Files with missing lines Coverage Δ
src/Serialization/Groups.jl 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HereAround HereAround merged commit 5192e9a into oscar-system:master Aug 15, 2025
34 checks passed
@HereAround HereAround deleted the MakeClausHappy-1 branch August 15, 2025 17:54
@lgoettgens lgoettgens changed the title Serialize FinGenAbGroupHom Add serialization of FinGenAbGroupHom Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes serialization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants