Skip to content

add float method#22

Open
jverzani wants to merge 2 commits into
s-celles:mainfrom
jverzani:float
Open

add float method#22
jverzani wants to merge 2 commits into
s-celles:mainfrom
jverzani:float

Conversation

@jverzani
Copy link
Copy Markdown
Contributor

@jverzani jverzani commented May 7, 2026

This adds a float method for GiacExpr which may be useful for generic programming. It covers some of the same surface as to_julia and unwrap_const so I'm not sure this direct approach is the best, but it seemed like the right way to do it to me.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 7, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.41%. Comparing base (84ef9da) to head (f553241).

Files with missing lines Patch % Lines
src/conversion.jl 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   79.50%   79.41%   -0.09%     
==========================================
  Files          22       22              
  Lines        2840     2852      +12     
==========================================
+ Hits         2258     2265       +7     
- Misses        582      587       +5     

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

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

Comment thread src/conversion.jl

julia> float(giac_eval("sin(2)"))
0.9092974268256817
"""
Copy link
Copy Markdown
Owner

@s-celles s-celles May 8, 2026

Choose a reason for hiding this comment

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

Triple backtick seems to be missing here (to close jldoctest)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! The windows error is interesting. Are there windows only limitations here? I naively didn't expect that.

Copy link
Copy Markdown
Owner

@s-celles s-celles May 9, 2026

Choose a reason for hiding this comment

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

This Windows error shouldn't be happening. We'll need to dig into them. I won't have time this weekend, but I'll look into it next week. If you come up with a solution before then, don't hesitate to send a commit.

Copy link
Copy Markdown
Owner

@s-celles s-celles May 10, 2026

Choose a reason for hiding this comment

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

@jverzani My agent found this....
Posting here for reference.
Some work is (will be) required!


Windows CI failure on PR #22 — investigation summary

Symptom

PR #22 — add float method (#22) (jverzani) fails the two windows-latest jobs (Julia 1 and Julia 1.10). All other jobs pass: 8924 tests pass, 1 doctest fails on Windows.

The failing doctest is in the new Base.float(::GiacExpr) method at src/conversion.jl:126:

julia> float(Giac.Commands.evalf(giac_eval("pi"), 100))
3.141592653589793238462643383279502884197169399375105820974944592307816406286198

  • Expected (Linux/macOS): 78-significant-digit BigFloat parsed from a multi-precision MPFR value.
  • Actual on Windows: 3.141592653589793 (16 digits, plain Float64).

The new float method dispatches on Giac.giac_type(ex). On Linux/macOS the evalf(pi, 100) result is _REAL (MPFR), taking the parse(BigFloat, string(ex)) branch. On Windows the result behaves like DOUBLE and takes the
convert(Float64, ex) branch — hence only 16 digits.

The PR is not at fault: it is the first thing in the package to exercise multi-precision evalf directly through a doctest, so it is the first thing to expose a pre-existing Windows-only limitation.

What was checked upstream

s-celles/giac (GIAC C++ source, pinned commit 194ff510)

  • meson.build:114 sets HAVE_LIBMPFR = 1 unconditionally, on every platform including MinGW.
  • All MPFR call sites on the evalf(pi, 100) path are not gated by __MINGW_H:
  • set_precision (src/gen.cc:2470) → for nbits ≥ 45 calls real_object(g, nbits).
  • m_pi(int nbits) (src/gen.cc:2570) → for nbits > 48 calls mpfr_const_pi.
  • accurate_evalf (src/gen.cc:2484) → no MinGW guard.
  • The only MPFR site guarded by !defined __MINGW_H is in chartab2gen (src/gen.cc:12228), which parses decimal literals with more than 14 digits. This affects giac_eval("3.14159…"), not evalf(pi, n).

→ The GIAC source does not disable multi-precision on Windows. The #ifdef WIN32 disables MPFR hypothesis is eliminated.

s-celles/libgiac-julia-wrapper (CxxWrap layer, pinned commit 49020792)

  • Gen::to_string() (src/giac_impl.cpp:529) calls g.print(&ctx) with a thread_local giac::context* (src/giac_impl.cpp:62).
  • Only a 1-arg giac_evalf(const Gen&) is exposed. The 2-arg Giac.Commands.evalf(g, 100) actually goes through invoke_cmd, which builds the string "evalf(, 100)" and hands it to the GIAC parser via giac_eval. Precision
    flows through the GIAC parser, not through the wrapper.
  • The wrapper's pinned commit is literally fix: mark Windows tests as continue-on-error (ABI mismatch) — pre-existing Windows ABI issues are documented at this layer.

BinaryBuilder recipes (Yggdrasil)

  • G/GIAC/build_tarballs.jl (v2.0.1): builds the s-celles/giac Meson fork, depends on MPFR_jll 4.1.1, GMP_jll 6.2.1. Same options on every platform.
  • L/libgiac_julia/build_tarballs.jl (v0.5.0): depends on GIAC_jll 2.0.1, libcxxwrap_julia_jll ~0.14, GMP_jll, MPFR_jll.

→ MPFR is in the dependency closure on Windows. A silent pkg-config failure is unlikely to leave HAVE_LIBMPFR=1 while the link fails — the link step would error out at build time.

Remaining hypotheses

The empirical signature on Windows must be one of:

#: H1
Diagnostic signature: giac_type == REAL, length(string(r)) ≈ 16
Likely cause: MPFR pretty-printer truncating output (printf locale, mpfr_printf on MinGW, or the Windows setlocale fallback in chartab2gen interfering)
────────────────────────────────────────
#: H2
Diagnostic signature: giac_type == DOUBLE
Likely cause: Multi-precision branch never taken at runtime — possibly the parser path evalf(_, n) not calling accurate_evalf because decimal_digits propagation differs under MinGW
────────────────────────────────────────
#: H3
Diagnostic signature: giac_type == REAL, internal precision low (mpfr_get_prec ≈ 53)
Likely cause: thread_local giac::context* in the wrapper not initialized properly under MinGW; default decimal_digits = 16 survives

Recommended next step: empirical isolation

A minimal probe (test/diagnose_mpfr.jl) has been prepared locally. It should be run on the Windows CI runner (cannot be reproduced on Linux/macOS) on a throwaway branch of s-celles/Giac.jl. It prints, for several variants
of evalf(pi, 100):

  • Giac.giac_type(r) (REAL / DOUBLE / FLOAT)
  • length(string(r)) and string(r) itself

Once the type and string length are observed, the fix location is uniquely determined:

  • H1 → fix in s-celles/giac (printer)
  • H2 → fix in the GIAC parser path / context handling
  • H3 → fix in s-celles/libgiac-julia-wrapper (context initialization)

Short-term workaround for PR #22

Independent of the upstream fix, PR #22 can be unblocked by replacing the evalf(_, 100) example in the doctest with one that does not depend on multi-precision (e.g., the existing evalf(giac_eval("23456789012345678901"))
which exercises the ZINT branch). The other examples in the doctest already cover the integer/double/complex/fraction/symbolic branches and are sufficient to demonstrate the new float method.


@s-celles
Copy link
Copy Markdown
Owner

An ugly fix might be (if we don't want to rebuild GIAC_jll, ...)

function Base.float(ex::GiacExpr)
    val = Giac.giac_type(ex)
    val == INT  && return convert(Float64, ex)
    val == ZINT && return parse(BigFloat, string(ex))
    val == CPLX && return Complefloat(real(ex)), float(imag(ex)))
    val == FRAC && return float(numer(ex)) / float(denom(ex))
    val == VECT && return [float(x) for x in ex]
    if val == DOUBLE || val == REAL || val == FLOAT
        s = string(ex)
        # MPFR reals are reported as DOUBLE on Windows (because of a bitfield ABI mismatch).
        # Disambiguate via string length: > Float64's ~17-digit representation
        # ⇒ parse as BigFloat.
        return length(s) > 18 ? parse(BigFloat, s) : convert(Float64, ex)
    end
    Giac.is_constant(ex) && return to_julia(Giac.Commands.evalf(ex, 16))
    throw(ArgumentError("Can't convert expression to a floating point type"))
end

but I don't like that much.

@jverzani
Copy link
Copy Markdown
Contributor Author

I'll let you decide. If you want me to update the PR, just let me know.

s-celles added a commit to s-celles/giac that referenced this pull request May 13, 2026
The historical bitfield layout of class gen:

  unsigned char type:5;
  unsigned char type_unused:3;
  signed char subtype;
  unsigned short reserved;

is laid out and ACCESSED differently across GCC versions. GCC fuses
adjacent bitfield writes (g.type = ...; g.subtype = ...;) into a
single wider store using version-dependent bit placements: GCC 8
and GCC 10 produce instruction sequences that disagree on which
bits of the resulting word hold `type`.

This isn't theoretical. In the libgiac_julia / Giac.jl stack on
Windows:
  - GIAC_jll is built with BinaryBuilder GCC 8
  - libgiac_julia_jll (the CxxWrap wrapper) is GCC 10
  - When libgiac writes a gen tagged _REAL (3) and the wrapper reads
    it, the wrapper sees _DOUBLE_ (1) instead.

A C++ probe (s-celles/libgiac-julia-wrapper#5)
ran on Linux/macOS/Windows in both modes confirmed: with the
bitfield, GCC's optimizer fuses writes; the byte layout is correct
ONLY when libgiac and consumer use the same GCC. With
GIAC_TYPE_ON_8BITS, type is a plain `unsigned char` at offset 0 —
no bitfield, no fusion, every compiler emits the same simple byte
store/load. The ABI becomes compiler-invariant.

Cost: 3 mantissa bits on inline doubles encoded in a gen (48 → 45).
sizeof(gen) is unchanged at 64 bits. Per giac's author this is the
historical default and the safer cross-compiler path. Override by
removing this block in dispatch.h if you need the legacy layout.

Fixes: s-celles/Giac.jl#22
Refs:  s-celles/libgiac-julia-wrapper#5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants