Changed xrange to range #30
Conversation
|
Thank you for your explanation and the commit! But, the master also contains 8 calls to xrange in test_feature_significance.py, see There was no specific intention to use xrange. Another colleague wrote the tests with a preference for xrange (We try to let different people write tests and code) |
Handling builtinsRelated to #8 (comment) Getting the codebase and tests aligned to run on both Python 2.7.x and However this would have ramifications on the zip function possibly as per The use of builtins as it stands now in i8_add_python3_support is as follows: tsfresh/tsfresh/feature_extraction/extraction.py - 1 builtins imports found However at the moment would possible to just handle builtins with a try and Perhaps there is scope to add As per http://python-future.org/compatible_idioms.html#imports-relative-to-a-package I am not certain of your thoughts on this. Porting to Python 3 is a lot of I am certain this port to Python 3 is going to raise lots of little questions, Does blue-yonder have any preferred methods? |
|
Thanks for your explanations. I will read into best practices about the importing issues with builtins. Actually, I do not have much experience with python 3.5.x. What do you think about the changes I made at the 'blue-yonder:i8_add_python3_support' branch so far? |
Substituting all builtins and past.builtinSubstituting all builtins and past.builtin with the following example from This try and except method was used to replace all builtins in the files listed This has not been added or merged to this pull request, it is just testing a PoC At some point I guess I should play with some timeseries data and the package |
|
Hopefully the above answers your question somewhat in terms of what I think about your changes so far. I have not actually executed any code yet. Been building testing and dev virtualenvs, githubbing and PoC'ing :) However that said. In my humble opinion I would suggest you start considering merging very small changes, very gradually so that i8_add_python3_support does not drift from master too much. From my limited experience with Python 3.5 although there are very small things, they do really influence design, structure and sometimes even performance. My gut feel (with very limited experience) is that the addition of Python 3 support may have some more overall ramifications, I reckon there is some probability :) That said, very small changes to your 2.7 master branch that SLOWLY bring that into line should help to not end up with 2 massively drifted branches. You people are now BUSY, you just got starred and social coding can be overwhelming. It is an entire new dimension :) And popular can be like an avalanche of things hitting you. So that is what I think about your changes in general :) I do not envy you :) I think maybe you should perhaps all take a moment to breathe and consider what you all think is important and what you all think the priorities should be, try not to let the all the social coding issues drive you, but the ones that you all think are important. After all a lot of things have not gone python 3 due to various constraints in the ecosysytem regarding pypy scipy numpy et al. So how important is 3? :) That said, have diffed master and i8_add_python3_support, maybe Python 3 support is not that vast amounts of change. I am happy to continue and try and see where it gets. Learning tsfresh backwards :) I hope that helps. |
* use python3 compatible print function * use absolute import for py3 support * in py3 one has to load reduce * check for basestring in column name * import range for py3 support * call list of range iterator * fixed standard library import * added builtin support for str, basestring, zip, object * class FeatureExtractionSettings inherits from baseobject * call list of iterator objects * change iter() to iteritems() * replaced filter by list comprehension * added future to requirements.txt * Changed xrange to range for #8 (#30) * csort range py3 compatible (#32) * Changed xrange to range for #8 * Modified csort due to range changing from list to type for #8 * Use assertItemsEqual py2 and assertCountEqual py3 Due to a change in unittest in as identified by @jneuff in #8 (comment) Semantically they appear to be the same and this fixes the related failing tests on Python 3.5 as described in the gist in #8 (comment) Adds a basic method to determine python version for now, only committing so that the new deeper unitest.assertEqual issue that now presents itself can be addressed. * pinned version of future package * added six to test-requirements.txt * use six for assertCountEqual unit testing * deleted some comments * use six for assertCountEqual to test feature_augmentor __dict__ will give iterator for properties * use six.assertCountEqual * use assertGreaterEqual to test subset * parameter have to be sorted in feature name fixes bug #29, features with multiple, but same parameters could end up with different names * added python 3.5.2 to .travis.yml closes #8
For #8
This fixes the use of xrange function in tests/test_feature_significance.py
xrange has been removed in Python 3 and range in Python 3 is now the same as
xrange in Python 2.7.x (with a caveat that there is evidence that certain
Python 3 range usages are NOT a fast as Python 2.7.x xrange).
Searching the code xrange is only used in tests/test_feature_significance.py,
however that said in the master version there are only 2 calls to xrange and in
the i8_add_python3_support branch version there were 8, so @MaxBenChrist I am
not certain if there was a specific intention there.
All tests pass on Python 3.5.2, without handling the builtins in Python 2.7.x it
is not possible to say that if these tests pass on 2.7.x, it is a small change
and range works as xrange only slower, seeing as very large timeseries data
could be handled, it seems pertainent just to flag it up.