HERA-Team / aipy

Astronomical Interferometry in PYthon (AIPY)
http://pypi.python.org/pypi/aipy
Other
43 stars 29 forks source link

python3 support #34

Closed cjordan closed 6 years ago

cjordan commented 6 years ago

As python2 support for things like numpy and astropy is going to be killed in the next couple of years, I think it makes sense for everyone to "jump ship". Personally, aipy is the only python package that ties me to python2. Are there any plans on updating the code?

From memory, attempting to install aipy with python3 raises an issue with a print statement, as it's still using the old syntax, but is easily fixed. However, I have no idea how much else needs to be changed. If it helps, I could have a go at changing print statements and other typical differences between 2 and 3.

pkgw commented 6 years ago

I've been doing most of the aipy maintenance duties lately and this has been something on my mind. Recently I ported over the NRAO CASA Python layer to work on both versions of Python, and porting the C modules was less work than I feared — I thought that would be the most challenging part.

The print statements are the most obvious compatibility problem between Python 2 and 3, but there's a wide range of things to deal with, especially when you have C code. And unfortunately the 2to3 tool will generate code that only runs on Python 3, whereas it is definitely going to be important for aipy to maintain compatibility with both major versions for a while yet.

Anyway, many of us in the Berkeley/aipy orbit expect to be using Python 2 for a while yet, so there hasn't been much pressure to port. I'd like to do it at some point, but I don't exactly have copious free time on my hands. You are extremely welcome to work towards this goal! I will be very thankful to merge any PRs to this end. (I recommend the six module and starting each file with from __future__ import absolute_import, division, print_statement.) But, be warned that there will be a lot of niggly bits to deal with, especially if you're not familiar with Python C extension modules.

cjordan commented 6 years ago

Thanks Peter. I don't have an awful amount of free time on my hands, but I'll keep this in mind when I'm looking to learn about C extension modules.

jaycedowell commented 6 years ago

This is something that I am interested in as well. I am working on porting my LSL library over to Python3 and aipy is the only dependency that is available under Python3. I should be able to have a PR for this fairly soon. The only real question I have is if there are any other tests apart from what is in the test suite that I could use to check the porting.

pkgw commented 6 years ago

@jaycedowell Wow! A PR adding Python 3 support would be very impressive and very welcome.

As for testing ... well, we certainly have a lot of production code that uses aipy that we could check for breakage. But that stuff doesn't come in the form of a formalized test suite, and a lot of that code is probably specific to Python 2 as well. But I can certainly guarantee that I will review any PRs quite carefully as well as checking the test suite results.

One request: it will help the review of any PRs if you break your work into small git commits — it'd be very appreciated if you're able to do so.

cjordan commented 6 years ago

My connectivity is very limited at the moment, so forgive me if I'm wrong, but my branch already has the python2-incompatible things (print statements mainly) fixed with future. Feel free to use that as a start.

I haven't found much time to work on this, but a project called py3c looks quite nice for the work talked about here. Not sure how feasible that is, though.

On 9 June 2018 at 07:56, Peter Williams notifications@github.com wrote:

@jaycedowell https://github.com/jaycedowell Wow! A PR adding Python 3 support would be very impressive and very welcome.

As for testing ... well, we certainly have a lot of production code that uses aipy that we could check for breakage. But that stuff doesn't come in the form of a formalized test suite, and a lot of that code is probably specific to Python 2 as well. But I can certainly guarantee that I will review any PRs quite carefully as well as checking the test suite results.

One request: it will help the review of any PRs if you break your work into small git commits — it'd be very appreciated if you're able to do so.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HERA-Team/aipy/issues/34#issuecomment-395921518, or mute the thread https://github.com/notifications/unsubscribe-auth/ABcYE8_bFk06vVqfPpAmTqfgoTeZ2-L_ks5t6w8RgaJpZM4SiD5- .

pkgw commented 6 years ago

Thanks @jaycedowell @cjordan ! Many aipy users are gathered together right now for a HERA busy week and we've talked ourselves into agreeing that now is that time to try to push this project over the finish line.

@jaycedowell it looks like you've been working on this quite recently. Are you near a stopping point at all? Even if your work isn't fully polished, I'd be eager to start working on closing this out: I'd make a branch that synthesizes your changes with those of @cjordan, then start tackling the C code and boring build-system stuff.

jaycedowell commented 6 years ago

Yes, I think that I am at a point where I have done all I can using the current test suite. My fork passes under Python 3.4.3 and my updated version of LSL passes all of its tests using this fork as well. I suspect that there are some problem lurking in the code that will pop out once they have some real world testing.

How do you want to do this? Do you want me to send in a PR to a particular branch?

pkgw commented 6 years ago

No need, I'll just start a personal branch that builds off of your work and merges in that of @cjordan. I'll CC you both on a pull request when I think the final bits are in place.

jaycedowell commented 6 years ago

Ok, sounds good. I should also point out a few things that are not strictly related to the Python3 support in my fork:

I think that is it.

cjordan commented 6 years ago

Hi all, just letting you know that I'm back at work, and once again well connected to the internet. Happy to help out with pull requests when the time comes, and I should be fairly prompt.

On 14 June 2018 at 05:53, jaycedowell notifications@github.com wrote:

Ok, sounds good. I should also point out a few things that are not strictly related to the Python3 support in my fork:

  • changed the astropy version to astropy>=1.0, <2.0 since I had trouble with both versions 2 and 3
  • added include_package_data = True and zip_safe = False to setup.py to get the Helmboldt catalog files to install
  • added a verbose keyword to the aipy.img.ImgW class initializer to suppress the "datum" output
  • added test suites for aipy.optimize and aipy.twodgauss to improve the testing coverage although I am not sure if I believe what is coming out of aipy.twodgauss.moments

I think that is it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HERA-Team/aipy/issues/34#issuecomment-397099910, or mute the thread https://github.com/notifications/unsubscribe-auth/ABcYE-r4M1pgbUL67J4BXHurXtVwuBAyks5t8YnbgaJpZM4SiD5- .

pkgw commented 6 years ago

OK, awesome!

What I have done is create a v3 branch in this repo that is marked as a development version of what will eventually be known as AIPY 3.0.0. You can install it now with pip as described in the README.

The idea is that we will shake down this version a bit and potentially make other breaking changes — @AaronParsons has expressed interest in some of these. Once we are happy with the branch, we can merge it into master and make an official AIPY 3.0.0 release.

cjordan commented 6 years ago

Excellent, cheers!

On Sat., 16 Jun. 2018, 01:14 Peter Williams, notifications@github.com wrote:

OK, awesome!

What I have done is create a v3 branch https://github.com/HERA-Team/aipy/tree/v3 in this repo that is marked as a development version of what will eventually be known as AIPY 3.0.0. You can install it now with pip as described in the README https://github.com/HERA-Team/aipy#python-3-and-aipy-3.

The idea is that we will shake down this version a bit and potentially make other breaking changes — @AaronParsons https://github.com/AaronParsons has expressed interest in some of these. Once we are happy with the branch, we can merge it into master and make an official AIPY 3.0.0 release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HERA-Team/aipy/issues/34#issuecomment-397686475, or mute the thread https://github.com/notifications/unsubscribe-auth/ABcYEwULVO__OexX5PnxH0ffDXRFdFqDks5t8-uCgaJpZM4SiD5- .

yurivict commented 5 years ago

Do you know what v3 branch is going to be merged? astropy is now python3-only, and aipy prevents astropy update because it is 2.7-only (as of version 2.1.12).

yurivict commented 5 years ago

Or, perhaps, you just need to make a release?