Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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 jdemeyer)

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)

ecl_iter.patch (5.6 KB) - added by nbruin 8 years ago.
trac_10743-reviewer.patch (1.3 KB) - added by kcrisman 8 years ago.
Apply after original patch
trac_10743-ecl-iter_p1.patch (5.6 KB) - added by jpflori 8 years ago.
Just corrected the small typo for double-float test and added ticket trac number to commit message; apply instead of original patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by nbruin

  • Description modified (diff)
  • Status changed from new to needs_review

Changed 8 years ago by nbruin

comment:2 Changed 8 years ago by nbruin

  • Owner changed from was to nbruin

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.

comment:3 follow-up: Changed 8 years ago by fbissey

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 nbruin

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 fbissey

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 jpflori

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 drkirkby

You should probably be aware of #10766 (an update of ECL), which has positive review. Also perhaps #10773 (an update of Maxima) which also has positive review.

Dave

comment:8 Changed 8 years ago by fbissey

I have been on this one and the maxima as ecl lib for a while Dave. They apply cleanly against #10766 and #10773. I imagine other people on these bugs are getting sick of the reminders.

comment:9 Changed 8 years ago by kcrisman

  • 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).

Changed 8 years ago by kcrisman

Apply after original patch

comment:10 Changed 8 years ago by kcrisman

  • 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 jpflori

  • 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 kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jean-Pierre Flori

Adding JP (if Trac will let me! third try) as reviewer due to the p1.

@JP: Just FYI, you can just do #10766 instead of #10766 to get the link automatically on Trac. Much easier!

comment:13 Changed 8 years ago by kcrisman

#10766

instead of

[http://trac.sagemath...10766 #10766

It was too smart in that last comment.

comment:14 Changed 8 years ago by jdemeyer

  • 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 jpflori

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 jpflori

  • Status changed from needs_work to needs_review

comment:16 Changed 8 years ago by jdemeyer

  • 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 jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.