Python 3 port #50
Python 3 port #50
Conversation
scimath/interpolate/fitting.py
Outdated
| indices, | ||
| 0, | ||
| numpy.Inf).astype( | ||
| numpy.int)) |
There was a problem hiding this comment.
Nit: stylistically, I'd break this up into two lines with a temporary to fit in the linewidth constraints.
indices = numpy.clip(indices, 0, numpy.Inf).astype(numpy.int)
indices = np.atleast_1d(indices)
There was a problem hiding this comment.
Thanks.. Fixed now..
scimath/units/function_signature.py
Outdated
|
|
||
| if defaults is not None: | ||
| # If there are keywords, then slice the variable list into | ||
| # positional and keyword arguments. | ||
| kw_count = len(defaults) | ||
| args = args_ordered[:-kw_count] | ||
| kw = dict(zip(args_ordered[-kw_count:], defaults)) | ||
| kw = dict(list(zip(args_ordered[-kw_count:], defaults))) |
There was a problem hiding this comment.
Don't need list here I believe.
|
|
||
| class QuantityTraitHandler ( TraitHandler ): | ||
| class QuantityTraitHandler (TraitHandler): |
There was a problem hiding this comment.
Autopep8 misses the extra space here. You may want to do a global search-replace for this sort of thing.
| @@ -1,13 +1,15 @@ | |||
| """ A Scalar is a Quantity object that limits data to floats. | |||
| """ | |||
|
|
|||
| from __future__ import absolute_import | |||
There was a problem hiding this comment.
Is this needed? Similarly in a few other places.
There was a problem hiding this comment.
https://www.python.org/dev/peps/pep-0328/#rationale-for-relative-imports
Apparently absolute imports are optional before 2.7. Given that the code works with 2.7 it is prob. not absolutely :) necessary.. but prob. good to keep until we drop support for < 2.7 ? not sure..
I am also partly to blame.. I thought this is a good opportunity to clean up some of the formatting and such.. |
|
@senganal How do you feel about the status of this PR? |
|
@jonathanrocher Thanks for the reminder.. IIRC I had paused on this as i had not tested the |
|
OK. Let me know if I can help... |
|
@jonathanrocher All the tests pass now on both py3 and py2 (on OSX). |
|
Since there is no CI on this project, I took the time to confirm that on Py2.7 and 3.6 both on OSX. I also passed the test suite on Win-Py2.7. I tried Win-Py3.6 but am running into compiler issues (EDM doesn't seem to give me a recent enough |
|
OK, I also tested py2.7 and py3.6 on Ubuntu 16.04 and tests are passing there too. HTH. |
|
@jonathanrocher Thanks for testing on linux.. I have tested on both OSX and Windows (py2 and py3). @corranwebster Comments have been addressed. I think this is good to go for now. |
|
Hi all, What is current state of the PR? I would like to get use this as a dev dependency in a project to increase Python 3 support. Let me know if there is anything that I can do to help. |
|
@JCorson I think its good to go.. Requires blessing from @corranwebster |
|
Thanks @senganal. FWIW I have tested on Windows Python 2.7 and 3.6. All tests passing. |
|
@corranwebster Want to give a final blessing on this? |
|
Go for it (this was totally off my radar). |
|
Already then... |
Current status: Tests pass on python 2 and python 3 (except those involving
pysideimports)TODO
Unfortunately, there seems to be a lot of broken code that is not being caught in the tests.. Not sure how to handle them.. Its prob. best to fix them in a separate PR and keep this focused on just the py2->3 conversion..