Opened 17 months ago
Closed 16 months ago
#28649 closed defect (fixed)
py3: flush output from FLINT error message
Reported by: | jhpalmieri | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.0 |
Component: | python3 | Keywords: | |
Cc: | chapoton, dimpase, egourgoulhon | Merged in: | |
Authors: | John Palmieri | Reviewers: | Dima Pasechnik, Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 9957855 (Commits, GitHub, GitLab) | Commit: | 99578558953d5fde55127dddbeceb86d33c395e0 |
Dependencies: | Stopgaps: |
Description (last modified by )
This is a second fix for the problem reported at #28334, the doctest failure in rings/polynomial/polynomial_rational_flint.pyx
. This fix does not rely on patching FLINT. See sage-devel discussion.
Change History (24)
comment:1 Changed 17 months ago by
- Description modified (diff)
comment:2 Changed 17 months ago by
- Branch set to u/jhpalmieri/flush-flint-msg
comment:3 Changed 17 months ago by
- Commit set to cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96
- Status changed from new to needs_review
New commits:
cd89b9d | trac 28649: flush C buffer
|
comment:4 Changed 17 months ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
good, thanks. Takes extra 0.5 sec on my laptop, but OK...
comment:5 Changed 17 months ago by
- Status changed from positive_review to needs_info
It may be possible to do this cross-platform in a relatively convenient way with ctypes anyway:
import ctypes libc = ctypes.CDLL(ctypes.util.find_library("c")) libc.fflush(0r)
seems to work, and looking at the source, it seems "find_library" does make an effort to be cross-platform. It's probably worth considering if this will do, since it's a lot lighter weight than running cython.
comment:6 Changed 17 months ago by
we can just add this flush function somewher into sagelib
comment:7 Changed 17 months ago by
Using Sage's own misc.compat.find_library
would probably be better:
-
src/sage/rings/polynomial/polynomial_rational_flint.pyx
diff --git a/src/sage/rings/polynomial/polynomial_rational_flint.pyx b/src/sage/rings/polynomial/polynomial_rational_flint.pyx index 73d1992656..aeb5a824e3 100644
a b cdef class Polynomial_rational_flint(Polynomial): 1176 1176 ... 1177 1177 RuntimeError: FLINT exception 1178 1178 1179 Flush the output buffer to get rid of stray output -- see 1180 :trac:`28649`:: 1181 1182 sage: from sage.misc.compat import find_library 1183 sage: import ctypes 1184 sage: libc = ctypes.CDLL(find_library("c")) 1185 sage: libc.fflush(0r) 1186 ... 1187 1179 1188 Test fractional powers (:trac:`20086`):: 1180 1189 1181 1190 sage: P.<R> = QQ[]
comment:8 Changed 17 months ago by
Both approaches work for me on OS X and on my Ubuntu virtual machine. Should they both work on Cygwin?
comment:9 Changed 17 months ago by
It looks like
libc = ctypes.CDLL(None)
works too. I have no idea why. (see https://eli.thegreenplace.net/2015/redirecting-all-kinds-of-stdout-in-python/)
comment:10 Changed 17 months ago by
Does https://stackoverflow.com/questions/49878901/how-does-ctypes-cdll-loadlibrarynone-work help? I'm worried about this on Cygwin, though.
comment:11 Changed 16 months ago by
Any opinions on which approach to take? I see three options:
- Keep the current branch. Slow, requires a functioning Cython compiler. (Is that guaranteed with binary releases?)
- Add the
cyflush
function to the Sage library so that it is precompiled.
- Take the
ctypes
approach from comment:17, withlibc = ctypes.CDLL(None)
.
Any opinions? I would like to get a solution to this problem merged, because there are people out there using non-Sage versions of FLINT.
comment:12 Changed 16 months ago by
I think 2 is the easiest. I don't see any benefits of using ctypes
here, whatsoever.
comment:13 Changed 16 months ago by
Any suggestions for where that function should go? polynomial_rational_flint.pyx
is one option, since it will be used there, but that's not a particularly natural place for anyone to find it later.
comment:14 Changed 16 months ago by
src/sage/misc/pager.py ?
comment:15 Changed 16 months ago by
or maybe misc/sage_ostools.pyx
comment:16 Changed 16 months ago by
I'm leaning toward either misc/misc_c.pyx or a new file cpython/cyflush.pyx.
comment:17 Changed 16 months ago by
- Commit changed from cd89b9d5a60a5e32ea3e6cafcfe5ed8d44ceaf96 to 461e0178180e869801ea2ed869c1647975701c73
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
461e017 | trac 28649: flush stray output from external library calls
|
comment:19 Changed 16 months ago by
- Commit changed from 461e0178180e869801ea2ed869c1647975701c73 to bc01c65367345d5a8ff8258731978a4708970ded
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bc01c65 | trac 28649: flush stray output from external library calls
|
comment:20 follow-up: ↓ 22 Changed 16 months ago by
the EXAMPLES:: content in cyflush is overindented.
comment:21 Changed 16 months ago by
- Commit changed from bc01c65367345d5a8ff8258731978a4708970ded to 99578558953d5fde55127dddbeceb86d33c395e0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9957855 | trac 28649: flush stray output from external library calls
|
comment:22 in reply to: ↑ 20 Changed 16 months ago by
comment:23 Changed 16 months ago by
- Reviewers changed from Dima Pasechnik to Dima Pasechnik, Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, good to go
comment:24 Changed 16 months ago by
- Branch changed from u/jhpalmieri/flush-flint-msg to 99578558953d5fde55127dddbeceb86d33c395e0
- Resolution set to fixed
- Status changed from positive_review to closed
As I said at #28334, the fix there can be kept independently of the change here, and it should be kept, since the patch to FLINT was accepted upstream. The one here is to handle the case when using non-patched versions of FLINT.