[Jython-bugs] [ jython-Patches-1608656 ] itertools module

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #1608656, was opened at 2006-12-04 18:33
Message generated for change (Comment added) made by h_eriksson
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1608656&group_id=12867

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Henrik Eriksson (h_eriksson)
Assigned to: Nobody/Anonymous (nobody)
Summary: itertools module

Initial Comment:
A patch for the itertools module in 2.3. The test coverage for the module is quite extensive, so I feel pretty confident that it works "as designed". One test fails, but this is a test of an implementation detail i CPython (reusing tuples). This is my first module port, so some things may not be "optimal" when it comes to the usage of internal Jython APIs. Feedback is very welcome. Oh, and I've actually never used svn before, so my file properties may be a bit off... 

'Henrik

----------------------------------------------------------------------

>Comment By: Henrik Eriksson (h_eriksson)
Date: 2007-02-13 21:34

Message:
Logged In: YES 
user_id=1652173
Originator: YES

You were right about the wrapping (of course). It's totally unnecessary. I
wasn't sure I liked the idea of using PyObject everywhere at first though,
but when I thought about it from a jython perspective instead of a java
perspective it makes sense :-)

----------------------------------------------------------------------

Comment By: Henrik Eriksson (h_eriksson)
Date: 2007-02-13 07:33

Message:
Logged In: YES 
user_id=1652173
Originator: YES

I can't verify it right now, but I think I need to wrap the iterable when
it isn't an iterable per say... Have a look at classes G through S in
test_itertools.py for examples. I'm pretty sure some of those won't work
without wrapping.

I'll have a closer look later.

----------------------------------------------------------------------

Comment By: Charles Groves (cgroves)
Date: 2007-02-13 07:02

Message:
Logged In: YES 
user_id=1174327
Originator: NO

I think you'll still get the PyIterator next element handling by
continuing to use your ItertoolsIterator class.  The underlying iterator
passed into ItertoolsIterator.nextElement will throw StopIteration when
it's done and you already handle that.

If using List vs. PyList significantly complicates the code, I wouldn't
worry about it.  The overhead isn't worth adding much complexity.

----------------------------------------------------------------------

Comment By: Henrik Eriksson (h_eriksson)
Date: 2007-02-12 08:39

Message:
Logged In: YES 
user_id=1652173
Originator: YES

This is exactly the kind of feedback I was looking for. Thanks!

Hmmm... it's been a while so I'm not 100% sure, but I think I wanted to
wrap it to be able to use PyIterator's next() method logic for throwing the
stored exception... might be overkill, I'll have a look at it.

You're right about toInt vs Py.py2int. I stared myself blind at getting
the tests to pass :-) 

I also agree about the PyList in cycle. It was quite handy to be able to
get a PyIterator from it that easily. But as you say, it has some
unnecessary overhead to it in this case. I'll make sure to fix that.

I'll get on it as soon as I get my new laptop (I recently switched
employer and haven't recieved one yet). Might take a week or two though :-(

I don't remember seeing any doctest failures but now I'm not sure... I'll
get back to you on that one.

----------------------------------------------------------------------

Comment By: Charles Groves (cgroves)
Date: 2007-02-11 07:42

Message:
Logged In: YES 
user_id=1174327
Originator: NO

This is some really nice work.  I love code that keeps from repeating
itself this well while remaining clear.  I do have a few concerns related
to internal Jython usage as you guessed might happen.

I'm not sure that you actually need the getIterator method.  I think you
can just use the PyObject returned by pyObj.__iter__() in there and take a
PyObject in ItertoolsIterator.nextElement and call __iternext__ on it
instead of next().  Is there another reason you're wrapping?

Instead of your toInt method, you can use Py.py2int.  I realize that's
going to break test_izip since it's expecting ValueErrors for most things,
but I think that's actually a bug in the test case.  A ValueError is for an
illegal value of the right type, whereas a TypeError as thrown by Py.py2int
is for the wrong type being passed in entirely as is the case with several
of the ValueError checks in test_izip.  It's not a big deal to change the
test since we're already going to have to for the existing CPython specific
failure.

Would it make more sense to use a Java ArrayList in cycle as saved rather
than a PyList?  A PyList has a bit of overhead in it compared to a regular
Java List and since it isn't exposed to Python code there's no reason to
use a PyList.

Do the doctests run for you?  They still don't for me with the operator
patch applied.  I get "AttributeError: class 'org.python.modules.operator'
has no attribute '__module__'" though that probably isn't your fault.

I hope the wait for a review hasn't put you off working on this.  If you
upload another patch with the issues I mentioned addressed, I'll happily
apply it.  I don't think we need to fix doctest to get it in, so just the
itertools issues are enough for me.

----------------------------------------------------------------------

Comment By: Henrik Eriksson (h_eriksson)
Date: 2007-01-10 10:11

Message:
Logged In: YES 
user_id=1652173
Originator: YES

The itertools tests used the pow function in operator. But it's not
directly related to itertools. 

----------------------------------------------------------------------

Comment By: Khalid Zuberi (kzuberi)
Date: 2007-01-10 09:58

Message:
Logged In: YES 
user_id=18288
Originator: NO

Wow, that's quite a bit of work. Dumb question #1: are the changes to
modules/operator.java at the end of the patch somehow related to itertools,
or are they part of some other set of fixes?

- kz

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=312867&aid=1608656&group_id=12867