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

Failing doctests in src/sage/modular/modform_hecketriangle #35273

Closed
2 tasks done
GMS103 opened this issue Mar 13, 2023 · 27 comments · Fixed by #35934
Closed
2 tasks done

Failing doctests in src/sage/modular/modform_hecketriangle #35273

GMS103 opened this issue Mar 13, 2023 · 27 comments · Fixed by #35934

Comments

@GMS103
Copy link
Member

GMS103 commented Mar 13, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Did you read the documentation and troubleshoot guide?

  • I have read the documentation and troubleshoot guide

Environment

- OS:  macOS (not relevant I think)
- Sage Version:  10.0.beta4

Steps To Reproduce

make ptestlong gives

sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/abstract_space.py  # 3 doctests failed
sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/readme.py  # 1 doctest failed
sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/space.py  # 2 doctests failed

Expected Behavior

All tests passed!

Actual Behavior

sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/abstract_space.py
**********************************************************************
File "src/sage/modular/modform_hecketriangle/abstract_space.py", line 1268, in sage.modular.modform_hecketriangle.abstract_space.FormsSpace_abstract.F_basis
Failed example:
    MF.F_basis(1)
Expected:
    q - 13071/(640000*d^2)*q^3 + O(q^4)
Got:
    q + (13071/(-640000*d^2))*q^3 + O(q^4)
**********************************************************************
File "src/sage/modular/modform_hecketriangle/abstract_space.py", line 1444, in sage.modular.modform_hecketriangle.abstract_space.FormsSpace_abstract.quasi_part_gens
Failed example:
    MF.quasi_part_gens(r=0)
Expected:
    [q - 34743/(640000*d^2)*q^3 + O(q^4), q^2 - 69/(200*d)*q^3 + O(q^4)]
Got:
    [q + (34743/(-640000*d^2))*q^3 + O(q^4), q^2 - 69/(200*d)*q^3 + O(q^4)]
**********************************************************************
File "src/sage/modular/modform_hecketriangle/abstract_space.py", line 1446, in sage.modular.modform_hecketriangle.abstract_space.FormsSpace_abstract.quasi_part_gens
Failed example:
    MF.quasi_part_gens(r=1)
Expected:
    [q - 9/(200*d)*q^2 + 37633/(640000*d^2)*q^3 + O(q^4),
     q^2 + 1/(200*d)*q^3 + O(q^4)]
Got:
    [q + (9/(-200*d))*q^2 + (-37633/(-640000*d^2))*q^3 + O(q^4),
     q^2 + 1/(200*d)*q^3 + O(q^4)]
**********************************************************************

sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/readme.py
**********************************************************************
File "src/sage/modular/modform_hecketriangle/readme.py", line 999, in sage.modular.modform_hecketriangle.readme
Failed example:
    MF.F_basis(1)
Expected:
    q - 13071/(640000*d^2)*q^3 + O(q^4)
Got:
    q + (13071/(-640000*d^2))*q^3 + O(q^4)
**********************************************************************

sage -t --long --warn-long 56.6 --random-seed=452785398975339820822274837493975258 src/sage/modular/modform_hecketriangle/space.py
**********************************************************************
File "src/sage/modular/modform_hecketriangle/space.py", line 399, in sage.modular.modform_hecketriangle.space.QuasiCuspForms.gens
Failed example:
    MF.gens()
Expected:
    [q - 17535/(262144*d^2)*q^3 + O(q^4),
     q^2 - 47/(128*d)*q^3 + O(q^4),
     q - 9/(128*d)*q^2 + 15633/(262144*d^2)*q^3 + O(q^4),
     q^2 - 7/(128*d)*q^3 + O(q^4),
     q - 23/(64*d)*q^2 - 3103/(262144*d^2)*q^3 + O(q^4),
     q - 3/(64*d)*q^2 - 4863/(262144*d^2)*q^3 + O(q^4),
     q - 27/(64*d)*q^2 + 17217/(262144*d^2)*q^3 + O(q^4)]
Got:
    [q + (17535/(-262144*d^2))*q^3 + O(q^4),
     q^2 - 47/(128*d)*q^3 + O(q^4),
     q + (9/(-128*d))*q^2 + (-15633/(-262144*d^2))*q^3 + O(q^4),
     q^2 - 7/(128*d)*q^3 + O(q^4),
     q - 23/(64*d)*q^2 - 3103/(262144*d^2)*q^3 + O(q^4),
     q - 3/(64*d)*q^2 - 4863/(262144*d^2)*q^3 + O(q^4),
     q - 27/(64*d)*q^2 + 17217/(262144*d^2)*q^3 + O(q^4)]
**********************************************************************
File "src/sage/modular/modform_hecketriangle/space.py", line 817, in sage.modular.modform_hecketriangle.space.CuspForms.gens
Failed example:
    MF.gens()
Expected:
    [q + 296888795/(10319560704*d^3)*q^4 + O(q^5),
     q^2 + 6629/(221184*d^2)*q^4 + O(q^5),
     q^3 - 25/(96*d)*q^4 + O(q^5)]
Got:
    [q + 296888795/(10319560704*d^3)*q^4 + O(q^5),
     q^2 + (-6629/(-221184*d^2))*q^4 + O(q^5),
     q^3 - 25/(96*d)*q^4 + O(q^5)]
**********************************************************************

Additional Information

Why plus/minus signs are not simplified in fractions?

@GMS103 GMS103 added the t: bug label Mar 13, 2023
@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 15, 2023

I see this with 10.0.beta4 but not 10.0.beta2 (both built today on the same machine), so it's not caused by recent homebrew upgrades, for example. (Unless I'm confused, which is certainly possible.)

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 16, 2023

Perhaps caused by #35045.

@jhpalmieri
Copy link
Member

I tried with a linux virtual machine and doctests passed, so somehow it seems to be platform-dependent.

@jhpalmieri
Copy link
Member

Marking as a blocker since we get failing doctests.

@mezzarobba
Copy link
Member

(Replying to #35045 (comment):)

I don't see how something like that can be platform-dependent. Have you compared the parents of the output on Linux and OSX, or checked if manually created expressions in the same parent failed to simplify too?

@jhpalmieri
Copy link
Member

I don't know the mathematics, so I don't know what to try. If I do a = MF.F_basis(1), then parent(a) and type(a) are the same on OS X and Linux, but their print representations are different. If you have access to an OS X machine, you can see this with 10.0.beta4 or later. The doctest failures are not there in 10.0.beta3, but they are after applying https://patch-diff.githubusercontent.com/raw/sagemath/sage/pull/35045.diff.

@jhpalmieri
Copy link
Member

Maybe there is some different system package, and this behavior is revealing that difference somehow.

@mezzarobba
Copy link
Member

If I do a = MF.F_basis(1), then parent(a) and type(a) are the same on OS X and Linux, but their print representations are different.

What about the parent of a.q_expansion()? Is it Power Series Ring in q over Fraction Field of Univariate Polynomial Ring in d over Integer Ring as on Linux, or something different (e.g., ... over RationalField)? How do the print representations of manually created elements of that parent look like? of its base ring?

@jhpalmieri
Copy link
Member

For both platforms, I see the same behavior:

sage: R = parent(a.q_expansion())
sage: R
sage: R.base_ring().inject_variables()
Defining d
sage: R(1/(-d))
1/-d

sage: R.inject_variables()
Defining q
sage: q + (13071/(-640000*d^2))*q^3
q + (13071/(-640000*d^2))*q^3

At least on OS X I see the same behavior in vanilla 10.0.beta3 — without the branch at #35045. (I haven't tried on linux.)

@jhpalmieri
Copy link
Member

The behavior looks very strange to me, even before #35045. With 10.0.beta3:

sage: from sage.modular.modform_hecketriangle.space import WeakModularForms, CuspForms
sage: MF = WeakModularForms(n=5, k=62/3, ep=-1)
sage: a = MF.F_basis(1)
sage: a
q - 13071/(640000*d^2)*q^3 - 26203/(1500000*d^3)*q^4 + O(q^5)
sage: R = parent(a.q_expansion())
sage: R.inject_variables()
Defining q
sage: R.base_ring().inject_variables()
Defining d

sage: q - 13071/(640000*d^2)*q^3 - 26203/(1500000*d^3)*q^4 + O(q^5)
q - 13071/(640000*d^2)*q^3 - 26203/(1500000*d^3)*q^4 + O(q^5)
sage: q + (13071/(-640000*d^2))*q^3 + (26203/(-1500000*d^3))*q^4 + O(q^5)
q + (13071/(-640000*d^2))*q^3 + (26203/(-1500000*d^3))*q^4 + O(q^5)

The last two expressions evaluate as equal, so why don't they print the same way? Maybe #35045 just changed how the expression is defined, from something along the lines of q - A/B to q + A/(-B), and this catches an apparently old behavior that the signs are not simplified to convert the second form to the first.

@jhpalmieri
Copy link
Member

Maybe it's Singular. When I use ./configure --with-system-singular=no, tests pass. On OS X, Homebrew includes Singular 4.3.2, whereas Sage includes 4.3.1.

@mezzarobba
Copy link
Member

mezzarobba commented Mar 22, 2023

The last two expressions evaluate as equal, so why don't they print the same way?

Sounds like #35221 (comment), but I don't understand why the issue occurs on some systems only then. (Edit: I had missed your comment about singular.)

Maybe #35045 just changed how the expression is defined

Yes, most probably.

@mezzarobba
Copy link
Member

Maybe it's Singular. When I use ./configure --with-system-singular=no, tests pass. On OS X, Homebrew includes Singular 4.3.2, whereas Sage includes 4.3.1.

Ok: not a bug, then?

@jhpalmieri
Copy link
Member

It causes doctests to fail, so it needs to be fixed.

@mezzarobba
Copy link
Member

Maybe... It is quite common to see a few failing doctests when using system packages for dependencies like pari, gap, or singular. I tend to view that as normal, though I agree that ideally it should be fixed (if we can do it by adapting the code or the tests, not by restricting the range of allowed versions of the dependency when the version for which the tests fail works fine otherwise).

@GMS103
Copy link
Member Author

GMS103 commented Mar 22, 2023

Whatever the cause, I do not see any reason to get
q + (13071/(-640000*d^2))*q^3 + O(q^4)
instead of
q - 13071/(640000*d^2)*q^3 + O(q^4).
This seems to me incoherent (when compared for instance with q^2 - 47/(128*d)*q^3 + O(q^4)).
Is it difficult to correct?

@mezzarobba
Copy link
Member

Is it difficult to correct?

Even if the difference in behavior between systems observed here is due to different versions of Singular, the fact that fractions are not normalized as one would expect looks a lot like the issue discussed in #35221 (comment).

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 22, 2023

A potential fix: replace doctests like

sage: MF.F_basis(1)
q - 13071/(640000*d^2)*q^3 + O(q^4)

with

sage: MF.F_basis(1) == q - 13071/(640000*d^2)*q^3 + O(q^4)
True

(Probably have to inject some variables first so that the right side is defined.) This is not altogether satisfying because I agree that we should see - 13071/(640000*d^2) rather than 13071/(-640000*d^2). But it would test the mathematics correctly.

@jhpalmieri
Copy link
Member

I tried creating some doctests of the form I mentioned in the previous comment, but I was unsuccessful. If someone else wants to try, I'm happy to test.

@vbraun
Copy link
Member

vbraun commented May 14, 2023

The doctests all have properly normalized fractions, so the fact that they aren't is imho indicative of a bigger problem and we shouldn't "fix" it by allowing the wrong normalization. We do have multiple OSX buildbots and they don't reproduce the issue, so its a Singular 4.3.2 bug or a bug in homebrew's packaging. Either way, it shouldn't block us from making a release.

@mezzarobba
Copy link
Member

This is just a guess, but I think: (1) there is a bigger problem, namely that rational fractions are not properly normalized in Sage in general; (2) but that problem is known and documented in other issues and the fact that it pops up here is an accident.

@jhpalmieri
Copy link
Member

Should we disallow this version of the system Singular?

@GMS103
Copy link
Member Author

GMS103 commented Jul 9, 2023

This issue seems related to Homebrew's Singular version 4.3.2p2, as it does not appear with Singular SPKG version 4.3.1p3. See #35833.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 11, 2023

The same version built from the updated SPKG (#35934) passes the tests.

The difference with the homebrew formula https://github.com/Homebrew/homebrew-core/blob/master/Formula/singular.rb#L56 is that we build Singular with FLINT.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 11, 2023

We could for example reject system singular if it does not use FLINT.
that's HAVE_FLINT in <singular/singularconfig.h>

Done now in #35934

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 11, 2023

Upstream does not test configurations without FLINT, it seems: https://github.com/Singular/Singular/blob/spielwiese/.github/workflows/runtests.yml#L15

@jhpalmieri
Copy link
Member

The same version built from the updated SPKG (#35934) passes the tests.

The difference with the homebrew formula https://github.com/Homebrew/homebrew-core/blob/master/Formula/singular.rb#L56 is that we build Singular with FLINT.

Does anyone here know anything about homebrew's development process? Would they be receptive to a pull request to bulid with FLINT?

@vbraun vbraun closed this as completed in bfb6b3c Sep 1, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants