-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add SageMath-FLINT bridge for number type conversion #39898
base: develop
Are you sure you want to change the base?
Add SageMath-FLINT bridge for number type conversion #39898
Conversation
This implements one-way conversion from SageMath types to FLINT types, allowing direct conversion of RR/CC objects to flint.arb/acb types.
This implements one-way conversion from SageMath types to FLINT types, allowing direct conversion of RR/CC objects to flint.arb/acb types.
Do use keyword https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests so that the issue is correctly linked to this pull request. |
Convert a SageMath matrix with RR/CC entries to FLINT matrix. | ||
|
||
Parameters: | ||
----------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use INPUT:
in our code base.
(Some AI not familiar with our code base may use whatever convention it learns. Not a problem per se, I'm happy if some AI solves my problem as long as the code is high quality, but not disclosing AI usage is dishonest.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct that I used AI assistance when drafting portions of this PR, particularly for structuring the documentation format. I should have disclosed this upfront, and I apologize for the oversight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll soon update the code to use SageMath's standard "INPUT:" convention instead of "Parameters:"
--------- | ||
>>> from sage.all import matrix, RR | ||
>>> M = matrix(RR, 2, 2, [1.1, 2.2, 3.3, 4.4]) | ||
>>> sage_matrix_to_flint(M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conversion must work out of the box with existing interface, unless there's very strong evidence it is impossible. (For RR
and CC
it should be possible, see my comment on the issue.)
Also need a # optional - python_flint
and the infrastructure behind it to add a new feature flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I attempted to create a monkey-patching solution that would modify Python-FLINT's initialization methods to directly handle SageMath types. However, I encountered a limitation: the flint.types.arb.arb
class has an immutable __init__
attribute that can't be directly replaced or modified.
Due to this constraint, I pivoted to creating explicit conversion functions instead. This approach works reliably but requires users to call the conversion functions rather than working automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I explained in the original issue
implement magic method
_mpf_
and_mpc_
onRR
andCC
object respectively
This should work at least for ℝ and ℂ, as you can figure by reading the source code. Unless I misread, then explain why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did attempt implementing mpf and mpc as you suggested in the original issue, but ran into difficulties with the Cython compilation of these methods in SageMath's core files.
@@ -0,0 +1,38 @@ | |||
.. nodoctest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why nodoctest?
# Real number examples at different precisions | ||
self.real_std = RR(1.234) | ||
self.real_high = RealField(200)(pi) | ||
self.real_exact = RR(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use doctest almost everywhere here instead of pytest.
Do read the existing code yourself and try to fit your contribution in before writing code.
It may be worth discussing this with @oscarbenjamin. We had some idle talk about interoperability between sage and python-flint (and maybe, via python-flint, other python packages using flint) at the last Flint workshop, and I think Oscar was open to adding dedicated interfaces to python-flint. |
Yes, absolutely. It is not clear to me what would be particularly useful for Sage though. Although python-flint cannot depend on Sage directly it can have things like Converting to strings seems like a bad idea as it will be inefficient and using decimal strings is especially inefficient for high precision floats. The proper interchange format between different floating point types should be integers like the |
Description
This PR implements a bridge interface between SageMath's numerical types and Python-FLINT's arbitrary precision types. It solves issue #39836 where direct conversion fails with code like:
The implementation provides three key functions:
sage_to_flint_arb
: Converts SageMath RealNumber to FLINT arbsage_to_flint_acb
: Converts SageMath ComplexNumber to FLINT acbsage_matrix_to_flint
: Converts SageMath matrices to FLINT matricesThe functions use string-based conversion through
mpmath
to preserve precision where possible, with fallback to direct conversion for edge cases. This approach allows high-precision values to be transferred correctly between the systems.Implementation Decisions
This PR implements one-way conversion (SageMath → FLINT) only, focusing on the most immediate need identified in issue #39836. The reverse direction (FLINT → SageMath) is not implemented in this PR for several technical reasons:
RealField_class
andComplexField_class
objects lack acreate_with_prec()
method that would be needed for precise control of output precision. This leads to errors like:The test suite has been designed to focus only on the SageMath → FLINT conversion path, with special handling for string representation differences that occur in high precision numbers.
Future Work
For a complete bidirectional solution, further work would be needed to properly handle precision in the FLINT → SageMath direction.
📝 Checklist
The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion (Interface with python-flint library #39836).
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.
⌛ Dependencies
None.