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

Allow coercion and conversion from AcbFieldElem and ComplexFieldElem to Float64 and ComplexF64 #2018

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

jamesnohilly
Copy link
Contributor

@jamesnohilly jamesnohilly commented Feb 5, 2025

This PR adds conversion functionality for Float64 and type-based conversion for ComplexF64 for AcbFieldElem and ComplexFieldElem. It follows the same structure as ArbFieldElem by splitting the conversion code into their types and using this in the appropriate convert.

This PR references oscar-system/Oscar.jl#4531.

@jamesnohilly jamesnohilly marked this pull request as draft February 5, 2025 14:49
@jamesnohilly jamesnohilly changed the title Add conversion to Float64 for AcbFieldElem Add conversions to Float64 Feb 5, 2025
@jamesnohilly jamesnohilly marked this pull request as ready for review February 5, 2025 14:58
Comment on lines +122 to +124
function convert(::Type{ComplexF64}, x::AcbFieldElem)
return ComplexF64(x)
end
Copy link
Member

Choose a reason for hiding this comment

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

@thofma so we have this convert method here because it existed before... We can remove it, but technically this is breaking. Thoughts? (I am fine with breaking ; or leaving it in; as long as we agree)

Comment on lines +126 to +128
function convert(::Type{Float64}, x::AcbFieldElem)
return Float64(x)
end
Copy link
Member

Choose a reason for hiding this comment

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

@thofma This convert method was added "for symmetry". But of course we can also remove it and only keep the convert to ComplexF64 (to retain compatibility), or drop both...

@thofma
Copy link
Member

thofma commented Feb 5, 2025

I guess we can keep the convert methods, since we already have a bunch for them.

re = _real_ptr(x)
t = _mid_ptr(re)
# 4 == round to nearest
v = @ccall libflint.arf_get_d(t::Ptr{arf_struct}, 4::Int)::Float64
Copy link
Member

Choose a reason for hiding this comment

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

We really should have constants for this. This is the declaration in C:

typedef enum
{
    ARF_RND_DOWN  = 0,
    ARF_RND_UP    = 1,
    ARF_RND_FLOOR = 2,
    ARF_RND_CEIL  = 3,
    ARF_RND_NEAR  = 4
}
arf_rnd_t;

We should have something similar, say

const ARF_RND_DOWN  = 0
const ARF_RND_UP    = 1
const ARF_RND_FLOOR = 2
const ARF_RND_CEIL  = 3
const ARF_RND_NEAR  = 4

somewhere in the code (for now we could create this manually, on the long run this could be created/updated from the FLINT code by a script @lgoettgens is working on).

@jamesnohilly please make a follow-up PR to add these constants somewhere (perhaps in src/arb/ArbTypes.jl for now), and use them at least in the code you are adding here (and possibly other places calling arf_get_d)

@fingolfin fingolfin changed the title Add conversions to Float64 Allow coercion and conversion from AcbFieldElem and ComplexFieldElem to Float64 and ComplexF64 Feb 6, 2025
@fingolfin fingolfin added enhancement release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Feb 6, 2025
@fingolfin fingolfin enabled auto-merge (squash) February 6, 2025 10:05
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.33%. Comparing base (b1cd279) to head (ce80066).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   88.31%   88.33%   +0.02%     
==========================================
  Files          98       98              
  Lines       36181    36203      +22     
==========================================
+ Hits        31953    31981      +28     
+ Misses       4228     4222       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fingolfin fingolfin merged commit 5b096ee into Nemocas:master Feb 6, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants