Skip to content

Fix R() when R is a BoundedRing#4429

Merged
thofma merged 1 commit intomasterfrom
th/boundeding
Jan 8, 2025
Merged

Fix R() when R is a BoundedRing#4429
thofma merged 1 commit intomasterfrom
th/boundeding

Conversation

@thofma
Copy link
Copy Markdown
Collaborator

@thofma thofma commented Jan 8, 2025

Before:

julia> B
(x <= (11, 0, 1//3))

julia> parent(B)()
ERROR: MethodError: Cannot `convert` an object of type ZZRingElem to an object of type Tuple{ZZRingElem, Int64, QQFieldElem}
The function `convert` exists, but no method is defined for this combination of argument types.
[...]

Stacktrace:
 [1] Oscar.GaloisGrp.BoundRingElem{Tuple{…}}(val::ZZRingElem, p::Oscar.GaloisGrp.BoundRing{Tuple{…}})
   @ Oscar.GaloisGrp ~/blabla/Oscar.jl/src/NumberTheory/GaloisGrp/GaloisGrp.jl:56

The Galois group code uses univariate evaluation, which we want to change in Nemocas/AbstractAlgebra.jl#1948 to use inplace operation like addmul!, which will call R().

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.36%. Comparing base (264490b) to head (acfd109).
Report is 74 commits behind head on master.

Files with missing lines Patch % Lines
src/NumberTheory/GaloisGrp/GaloisGrp.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4429      +/-   ##
==========================================
+ Coverage   84.34%   84.36%   +0.02%     
==========================================
  Files         663      663              
  Lines       87788    87805      +17     
==========================================
+ Hits        74042    74076      +34     
+ Misses      13746    13729      -17     
Files with missing lines Coverage Δ
src/NumberTheory/GaloisGrp/GaloisGrp.jl 79.50% <0.00%> (ø)

... and 5 files with indirect coverage changes

Copy link
Copy Markdown
Contributor

@fieker fieker left a comment

Choose a reason for hiding this comment

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

If it works... This is the correct idea...
Using ring confirmance will be fun as boundrings are no rings....

@thofma thofma merged commit 2fbf70b into master Jan 8, 2025
@thofma thofma deleted the th/boundeding branch January 8, 2025 19:43
@aaruni96 aaruni96 added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Jan 30, 2025
@fingolfin fingolfin changed the title fix: R() for R::BoundedRing Fix R() when R is a BoundedRing Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: number theory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants