-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python3 #7
Python3 #7
Conversation
Please review this. The goal of this PR is to prepare this python code to be merged into the nupic.cpp repo. I recommend reviewing each commit, since the first commit is computer generated and very large & repetitive. Notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on py2/3 compatibility! Many thanks!
since the first commit is computer generated and very large & repetitive.
reviewed all, but only skimmed through the 1st, I think we can reasonably trust 2to3
Capnp is still here
ok, can remain. It's wrapped with try-import-catch. I'd suggest leaving it there so the diff to numenta's is not necessarily inflated.
I did not test with python2
please do, or I'll after Wed, currently swamped at work.
CI is broken with error: /Users/travis/.travis/functions: line 104: virtualenv: command not found
ditto, I can fix it later. Or fix package install in CI, maybe it'll hint the name of package, or you have to search CI's OS + virtualenv
@dkeeney might know if someone else already also did the py2->3 migration? (CHenning?)
yes, @chhenning did the py2->3 migration. There may have been changes to the numenta code since. Also note that Windows cannot do Python2 (cannot create the bindings for it) and @chhenning was a Windows user so he started with both Python2 and 3 but ended up only checking out Python3. See https://github.com/chhenning/nupic (nupic/src/python/) |
This is way cool 👍 |
What are other ways to help you with the migration? By the way how much test coverage do we have for testing nupic, any ideas? |
Hey @pepedocs , you're just in time! We're in the process of
...so any help is welcome and will be very needed.
Then the key focus is on #4 |
Sorry everyone, I misunderstood the how the unit tests worked and they weren't all being run. The real error count for the unit tests is off the charts: I also think I submitted this PR to the wrong repository. For our long term goals, I think a better work-flow would be to:
|
Great job!
What do you mean by wrong repo, isn't this the right repo? Have you tried installing this using py2 and py3 and fixed all the install errors? |
My goals is to merge this repository into the nupic.cpp repository. Since these repositories do the same things, it doesn't make sense to split them up. If we merge them, then we can have the python code and the bindings which it relies on all in the same place. It also brings the python test cases into the same place as the C++ algorithms they test. The duplicate code which is implemented in both C++ and pure-Python benefits because we can more easily test that they're the identical. I talked about this here https://discourse.numenta.org/t/survey-features-api-compatibility/5344 |
I propose that all .py code from this repository be placed in a top level folder 'python'.
This would be in parallel to the bindings and library code
|
Nitpick: how about |
@pepedocs did you have a chance to validate it "works" (the same as master) here in py2, py3? If so, we'll merge this branch. |
Well as a quick acceptance test I tried installing this using py2 and I got syntax errors such as,
On the other hand for py3 install I found an error that said it cannot find the nupic.bindings which I believe I need to install for py3. Which nupic.bindings is for py3 by the way? Please mention your test methods if you can so I can try them in my workspace. |
I used the python3 bindings from
Test Methods for nupic.cpp:
|
I'm getting errors when building nupic.cpp using;
Errors are in SDR_Metrics*
|
I can't reproduce the error but I did some google searching and I think I have a fix. I made a new branch with the fix. When you have the time, please give it a try and tell us if it works. Thanks for testing out this new repo, and especially thanks for reporting the bugs in it! |
Perhaps a little explanation of what those commands do would be useful.
The python bindings that are made available for import are
This should all be covered in the README.md. If you have problems with any of this please let me know, even if minor, since you are some of the first to try out the new installation procedures. |
Thanks everyone for the wonderful community support! <3 |
Checked out branch Do we have a CI for nupic.cpp? Let me know if you need more testing.
|
moving the build issue discussion to htm-community/htm.core#274, please follow up only there. |
I'll start with builds/install then proceed to code changes when possible. For this PR, I can confirm that Do we need to support py2? |
Great, thanks for testing. |
Ok so I've fixed the syntax errors both for py2/3. Now I'm still seeing other kinds of test errors. If any of you are interested at fixing these errors by continuing what I've started please let me know so I will commit my changes for this PR. I will continue fixing the errors but I'll be slow as I'm only doing this on my spare time. |
please do commit your changes continuously and update the PR, so if anyone picks up, no work is lost.
if those errors are caused by the broken tests, and not the py2->3 migration, we'll merge this PR. |
I don't think I have rights to update/push changes to this PR. |
please make your fork of this repo (or numenta/nupic), push the local changes and then open a PR to this branch, I'll merge it. |
Clarification: Please open a PR to |
@ctrl-z-9000-times My changes are on top of yours, did you mean I have to create a new PR with your changes plus mine? Sorry I'm a bit confused. |
@pepedocs I've opened the PR, #8 |
This PR makes this repo work for python3
Used 2to3 tool, and then fixed most of the remaining issues.