#10743 closed enhancement (fixed)
Add iterator protocol to EclObject
Reported by: | nbruin | Owned by: | nbruin |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | interfaces | Keywords: | ECL lisp ecllib |
Cc: | Merged in: | sage-4.7.alpha3 | |
Authors: | Nils Bruin | Reviewers: | Karl-Dieter Crisman, Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Experience teaches that EclObjects? wrapping LISP lists would often be convenient to iterate over in list comprehensions and for loops. Pythonizing the EclObject? would work recursively, which is often not what is required: you just want to work with EclObjects? wrapping the members of the list. The iterator protocol seems to yield a convenient and well-defined interface. Examples:
sage: from sage.libs.ecl import * sage: [i for i in EclObject("(1 2 3)")] [<ECL: 1>, <ECL: 2>, <ECL: 3>] sage: [i for i in EclObject("(1 2 . 3)")] [<ECL: 1>, <ECL: 2>, <ECL: 3>] sage: [i for i in EclObject("NIL")] [] sage: [i for i in EclObject("T")] Traceback (most recent call last): ... TypeError: ECL object is not iterable
Depends on #10766.
Apply trac_10743-ecl-iter_p1.patch and trac_10743-reviewer.patch
See also #7377.
Attachments (3)
Change History (20)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Owner changed from was to nbruin
comment:3 follow-up: ↓ 4 Changed 8 years ago by
That's strange, in that patch everything seems to be moved one column to the right compared to the original sources. Just plain patch didn't like it. I haven't tried with mercurial patch but I would be surprised if it worked.
comment:4 in reply to: ↑ 3 Changed 8 years ago by
Replying to fbissey:
I haven't tried with mercurial patch but I would be surprised if it worked.
It does apply cleanly to 4.6.1 and the patchbot was happy with the patch too (although the patchbot seems to be down at the moment)
sonny devel/sage$ ../../sage -hg log | head changeset: 15205:f24ce048fa66 tag: tip user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Tue Jan 11 08:10:26 2011 +0100 summary: 4.6.1 changeset: 15204:173ae89d6291 user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Tue Jan 11 04:51:03 2011 +0100 summary: Added tag 4.6.1 for changeset a05facfc3390 sonny devel/sage$ ../../sage -hg diff sonny devel/sage$ ../../sage -hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10743/ecl_iter.patch adding ecl_iter.patch to series file sonny devel/sage$ ../../sage -hg qpush applying ecl_iter.patch now at: ecl_iter.patch sonny devel/sage$ ../../sage -hg log | head changeset: 15206:d3554f7e41db tag: ecl_iter.patch tag: qbase tag: qtip tag: tip user: Nils Bruin <nbruin@sfu.ca> date: Fri Feb 04 14:19:57 2011 -0800 summary: Add iterator protocol to EclObject changeset: 15205:f24ce048fa66 sonny devel/sage$ ../../sage -bt sage/libs/ecl.pyx ---------------------------------------------------------- sage: Building and installing modified Sage library files. Installing c_lib scons: `install' is up to date. Updating Cython code.... Building modified file sage/libs/ecl.pyx. python `which cython` --embed-positions --directive cdivision=True,autotestdict=False -I/usr/local/sage/4.6.1/devel/sage-test -o sage/libs/ecl.c sage/libs/ecl.pyx sage/libs/ecl.pyx --> /usr/local/sage/4.6.1/local//lib/python/site-packages//sage/libs/ecl.pyx Time to execute 1 commands: 3.12507390976 seconds Finished compiling Cython code (time = 3.74062800407 seconds) running install running build running build_py running build_ext building 'sage.libs.ecl' extension gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -O3 -Wall -Wstrict-prototypes -fPIC -I/usr/local/sage/4.6.1/local/include/ecl/ -I/usr/local/sage/4.6.1/local//include -I/usr/local/sage/4.6.1/local//include/csage -I/usr/local/sage/4.6.1/devel//sage/sage/ext -I/usr/local/sage/4.6.1/local/include/python2.6 -c sage/libs/ecl.c -o build/temp.linux-i686-2.6/sage/libs/ecl.o -w gcc -pthread -shared build/temp.linux-i686-2.6/sage/libs/ecl.o -L/usr/local/sage/4.6.1/local//lib -L/usr/local/sage/4.6.1/local/lib -lcsage -lecl -lstdc++ -lntl -lpython2.6 -o build/lib.linux-i686-2.6/sage/libs/ecl.so Time to execute 1 commands: 0.438040018082 seconds Total time spent compiling C/C++ extensions: 0.49472618103 seconds. running install_lib copying build/lib.linux-i686-2.6/sage/libs/ecl.so -> /usr/local/sage/4.6.1/local/lib/python2.6/site-packages/sage/libs running install_egg_info Removing /usr/local/sage/4.6.1/local/lib/python2.6/site-packages/sage-0.0.0-py2.6.egg-info Writing /usr/local/sage/4.6.1/local/lib/python2.6/site-packages/sage-0.0.0-py2.6.egg-info sage -t "devel/sage-test/sage/libs/ecl.pyx" [2.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 2.3 seconds
comment:5 Changed 8 years ago by
ok, that's still quite weird, I have never had a patch like that before. I wonder if google chromium did something to it at download time. In any case sorry for the noise.
comment:6 Changed 8 years ago by
The patch applied cleanly on 4.6.2alpha4.
I got one minor doctest failure running "make ptest" (I'm using the new ECL 11.1 and Maxima 5.23 packages, maybe something changed with the new ECL):
The following tests failed: sage -t -force_lib devel/sage/sage/libs/ecl.pyx # 1 doctests failed ---------------------------------------------------------------------- Total time for all tests: 2519.8 seconds make: *** [ptest] Erreur 128 [jp@napoleon]% sage -t -force_lib devel/sage/sage/libs/ecl.pyx ~/sage/sage-4.6.2.alpha4 zsh: command not found: sage [jp@napoleon]% ./sage -t -force_lib devel/sage/sage/libs/ecl.pyx ~/sage/sage-4.6.2.alpha4 sage -t -force_lib "devel/sage/sage/libs/ecl.pyx" ********************************************************************** File "/home/jp/sage/sage-4.6.2.alpha4/devel/sage/sage/libs/ecl.pyx", line 415: sage: a Expected: <ECL: 9.999999999999999E39> Got: <ECL: 9.999999999999999e39>
comment:7 Changed 8 years ago by
comment:8 Changed 8 years ago by
comment:9 Changed 8 years ago by
- Reviewers set to Karl-Dieter Crisman
Somewhat surprisingly, I could understand all of this code, and it makes perfect sense. Tests work out fine, obviously, though I don't know if any edge cases have been tried, since I am not sure what the edge cases would be. My guess is that this is used fairly heavily in #7377, is that correct?
My only request is that there might be a doctest to show the list and tuples working directly, like so:
sage: [i for i in EclObject([1,2,3])] [<ECL: 1>, <ECL: 2>, <ECL: 3>] sage: [i for i in EclObject((1,2,3))] [<ECL: 1>, <ECL: 2>, <ECL: 3>]
I realize that combining the doctests from EclObject and this module amount to the same thing, but it would be very helpful for someone wanting to understand the Ecl interface better.
I'm going to attach a reviewer patch for this, with the understanding that if for some reason this causes #7377 patches to not apply correctly, the positive review for the current patch would remain (and I'd open a separate ticket for this).
comment:10 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
Apply attachment:trac_10743-ecl-iter_p1.patch
If it doesn't screw up #7377 applying properly (which it shouldn't, as #7377 only touches ecl.pyx in one small place far from here), also apply attachment:trac_10743-reviewer.patch
If for some reason it does cause problems, please open a new ticket for this minor improvement.
comment:11 Changed 8 years ago by
- Description modified (diff)
Just wanted to add that the minor change in the p1 patch has been introduced by the new ECL package so this should depend on #10766 to pass doctests.
comment:12 Changed 8 years ago by
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jean-Pierre Flori
comment:13 Changed 8 years ago by
#10766
instead of
[http://trac.sagemath...10766 #10766
It was too smart in that last comment.
comment:14 Changed 8 years ago by
- Status changed from positive_review to needs_work
Change the commit message of the first patch such that it contains the ticket number.
Changed 8 years ago by
Just corrected the small typo for double-float test and added ticket trac number to commit message; apply instead of original patch
comment:15 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:16 Changed 8 years ago by
- Merged in set to sage-4.7.alpha3
- Resolution set to fixed
- Status changed from needs_review to closed
comment:17 Changed 8 years ago by
- Description modified (diff)
Patch also fixes a documentation issue: printing of floats in LISP is dependent on an environment setting, and Maxima changes that setting when loaded (sets doubles to be the default rather than single precision floats). Since #7377 preloads Maxima in sage.libs.ecl, it changes the printing of floats and breaks a doctest in EclObject?. This patch also changes the doctest so that the environment is fixed, which means the result of the doctest is not dependent on preloading Maxima anymore.