Skip to content

Add isinteger, is_rational and is_integral for QQAbFieldElem#5126

Merged
fingolfin merged 3 commits intooscar-system:masterfrom
SoongNoonien:int_rat
Jul 18, 2025
Merged

Add isinteger, is_rational and is_integral for QQAbFieldElem#5126
fingolfin merged 3 commits intooscar-system:masterfrom
SoongNoonien:int_rat

Conversation

@SoongNoonien
Copy link
Copy Markdown
Member

This depends on thofma/Hecke.jl#1923 and thofma/Hecke.jl#1922.

fixes #5096

@joschmitt joschmitt added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: number theory needs hecke update labels Jul 17, 2025
Comment thread test/Rings/AbelianClosure.jl Outdated
Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>
@lgoettgens
Copy link
Copy Markdown
Member

@SoongNoonien could you already adapt the Hecke compat bound in the Project.toml to 0.37.2?

Comment on lines +937 to +941
@doc raw"""
isinteger(a::QQAbFieldElem)

Return whether $a$ is an integer.
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we really add this kind of docstring? Isn't this already covered by the Julia's primary docstring for isinteger? In fact looking at the output of ?isinteger in the REPL I think we could basically remove all of them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we need some concerned effort to remove some redundant docstrings (I am also one of the offenders). It is easier now since we can include docstrings multiple times in the documentation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough. I am probably as well. And some of them are not trivial to remove because e.g. the Nemo doc includes several of them (but I think in all cases this could be avoided by inserting some general text like "For this ring the following things are implemented: +, -, isinteger, "

Anyway, no reason to hold up this PR I think.

@thofma
Copy link
Copy Markdown
Collaborator

thofma commented Jul 18, 2025

If JuliaRegistries/General#134995 is merged, someone can open/close this PR here.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.91%. Comparing base (ac87163) to head (c6f4ffa).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5126   +/-   ##
=======================================
  Coverage   84.90%   84.91%           
=======================================
  Files         706      706           
  Lines       95627    95631    +4     
=======================================
+ Hits        81191    81201   +10     
+ Misses      14436    14430    -6     
Files with missing lines Coverage Δ
src/Rings/AbelianClosure.jl 93.83% <100.00%> (+0.03%) ⬆️

... 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.

@fingolfin fingolfin merged commit d7ac425 into oscar-system:master Jul 18, 2025
39 of 66 checks passed
@SoongNoonien SoongNoonien deleted the int_rat branch November 3, 2025 14:27
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.

Add isinteger/is_integer, is_rational and is_integral/is_algebraic_integer for QQAbFieldElem

5 participants