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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silence initialization of giac #35513

Merged
merged 2 commits into from
Apr 23, 2023

Conversation

tornaria
Copy link
Contributor

When sage.libs.giac.giac is imported for the first time, some noise is output to stderr. This is annoying and complicates doctesting as ... has to be added to the first time giac is used.

This output comes from the line

Pygen('add_language(1)').eval()  # Add the french keywords in the giac library language.

which doesn't seem to be necessary.

This commit removes the line, and fixes a few doctests which where expecting some output when using libgiac for the first time.

Also label # long time a doctest in src/sage/calculus/functional.py which takes 6-7 seconds. Doing this in the first place motivates the current change as the initialization noise would happen in different tests depending on whether --long or not is used.

Same for a doctest in src/sage/calculus/tests.py, and one in src/sage/symbolic/integration/integral.py. A test in src/sage/libs/giac/giac.pyx was marked # random but after this fix it seems to work ok.

馃摑 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • There are existing tests covering the changes.

@tornaria tornaria requested a review from videlec April 15, 2023 03:43
@videlec
Copy link
Contributor

videlec commented Apr 15, 2023

You have a doctest failure

sage -t --random-seed=272486796797018539112758668052593627245 sage/symbolic/relation.py
**********************************************************************
File "sage/symbolic/relation.py", line 932, in sage.symbolic.relation.solve
Failed example:
    solve([(2/3)^x-2], [x], algorithm='giac')
Expected:
    [[-log(2)/(log(3) - log(2))]]
Got:
    Warning, argument is not an equation, solving (2/3)^sageVARx-2=0
    [[-log(2)/(log(3) - log(2))]]
**********************************************************************
1 item had failures:
   1 of 124 in sage.symbolic.relation.solve
    [395 tests, 1 failure, 12.41 s]

When sage.libs.giac.giac is imported for the first time, some noise is
output to stderr. This is annoying and complicates doctesting as `...`
has to be added to the _first_ time giac is used.

This output comes from the line

    Pygen('add_language(1)').eval()  # Add the french keywords in the giac library language.

which doesn't seem to be necessary.

This commit removes the line, and fixes a few doctests which where
expecting some output when using libgiac for the first time.

Also label `# long time` a doctest in `src/sage/calculus/functional.py`
which takes 6-7 seconds. Doing this in the first place motivates the
current change as the initialization noise would happen in different
tests depending on whether `--long` or not is used.

Same for a doctest in `src/sage/calculus/tests.py`, and one in
`src/sage/symbolic/integration/integral.py`. A test in
`src/sage/libs/giac/giac.pyx` was marked `# random` but after this fix
it seems to work ok.
@tornaria tornaria force-pushed the giac-silence_initialization branch from 0d5f313 to 599ae9f Compare April 15, 2023 21:49
@tornaria
Copy link
Contributor Author

You have a doctest failure

sage -t --random-seed=272486796797018539112758668052593627245 sage/symbolic/relation.py
**********************************************************************
File "sage/symbolic/relation.py", line 932, in sage.symbolic.relation.solve
Failed example:
    solve([(2/3)^x-2], [x], algorithm='giac')
Expected:
    [[-log(2)/(log(3) - log(2))]]
Got:
    Warning, argument is not an equation, solving (2/3)^sageVARx-2=0
    [[-log(2)/(log(3) - log(2))]]
**********************************************************************
1 item had failures:
   1 of 124 in sage.symbolic.relation.solve
    [395 tests, 1 failure, 12.41 s]

It should be fixed now, my bad...

This change was not intended to go in. The ... in this doctest corresponds to a warning, which doesn't show up in my version of giac (1.9.0-43) but it still shows up in the version bundled with sage.

I only meant to remove a few ... which correspond to the initialization of giac that this PR is silencing.

@kiwifb
Copy link
Member

kiwifb commented Apr 15, 2023

@fchapoton you are credited with the change adding "French keywords" in 7d3fd94 . What was your motivation with that?

@tornaria
Copy link
Contributor Author

@fchapoton you are credited with the change adding "French keywords" in 7d3fd94 . What was your motivation with that?

No, that's just a style change. I tracked that line all the way to the original commit that includes giacpy into sage: d095c37.

Here is the line:

Pygen('add_language(1)').eval(); # Add the french keywords in the giac library language.

The ticket for this is #29171.

Now, this code originates in https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/, more precisely:
https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/-/blob/master/giacpy_sage.pyx#L6386.

And looking at the giacpy history, the file giacpy_sage.pyx is created as a 5636-line file which already contains this line in https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/-/commit/7b1bf00d364d52d0b3f74f72c0d21f9e95375fa6.

@kiwifb
Copy link
Member

kiwifb commented Apr 16, 2023

Thanks for doing the archeology. It is just so bizarre to single out French unless there is a deep reason to it. We definitely have to expunge this.

Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

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

The bots are happy and everything that needed sorting has been. Let's move on.

This `...` corresponds to a warning in the version of giac bundled with
sage, but the warning is removed in current version of giac (tested with
1.9.0-43).

Before this `...` was ok since it was matching giac initialization, but
after the last commit this is no longer the case.

Placing the `...` in the same line as the output makes it optional.
@tornaria
Copy link
Contributor Author

@kiwifb I had to add a small fix to support new giac. Can you verify that everything is still ok for you?

Fix doctest so it works with old and new giac.

This `...` corresponds to a warning in the version of giac bundled with
sage, but the warning is removed in current version of giac (tested with
1.9.0-43).

Before this `...` was ok since it was matching giac initialization, but
after the last commit this is no longer the case.

Placing the `...` in the same line as the output makes it optional.

@kiwifb
Copy link
Member

kiwifb commented Apr 17, 2023

I'll try to test it later today.

@github-actions
Copy link

Documentation preview for this PR is ready! 馃帀
Built with commit: 656b940

@kiwifb
Copy link
Member

kiwifb commented Apr 21, 2023

Well Volker is already merging it. It does work OK with 1.9.0-43. Although, I am not pushing for either -41 or -43. Both give me a segfault in pari when running the giac testsuite and I haven't really had time to investigate further.

@tornaria
Copy link
Contributor Author

Well Volker is already merging it. It does work OK with 1.9.0-43. Although, I am not pushing for either -41 or -43. Both give me a segfault in pari when running the giac testsuite and I haven't really had time to investigate further.

Try this: https://github.com/void-linux/void-packages/blob/master/srcpkgs/giac/patches/giac-pari-2.15-test.patch

I'm running 1.9.0-43.

@vbraun vbraun merged commit 20d2edd into sagemath:develop Apr 23, 2023
6 of 7 checks passed
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 23, 2023
@kiwifb
Copy link
Member

kiwifb commented Apr 25, 2023

Well Volker is already merging it. It does work OK with 1.9.0-43. Although, I am not pushing for either -41 or -43. Both give me a segfault in pari when running the giac testsuite and I haven't really had time to investigate further.

Try this: https://github.com/void-linux/void-packages/blob/master/srcpkgs/giac/patches/giac-pari-2.15-test.patch

I'm running 1.9.0-43.

Thanks heaps. Helps with 1.9.0-45 too, I just tested it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants