Skip to content
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

Python3new #90

Closed
wants to merge 41 commits into from
Closed

Python3new #90

wants to merge 41 commits into from

Conversation

scottransom
Copy link
Owner

This is work-in-progress, based on a bunch of work by @matteobachetti ( see #72 ) to allow both Python 2 and Python 3 in PRESTO.

@scottransom scottransom mentioned this pull request Jun 5, 2018
3 tasks
@paulray
Copy link
Contributor

paulray commented Jul 10, 2018

Any timescale for when this might get merged?

@scottransom
Copy link
Owner Author

@paulray Not quite sure. A lot of the basic infrastructure code has been changed and at least basically checked, but most of the python applications have not yet been. That will take some effort. I'm hoping to work on it some more this week and next. I'd like to have it all merged in by the end of the month.

If there is any code that you use that you might be able to easily test, that would be appreciated!

@paulray
Copy link
Contributor

paulray commented Jul 10, 2018

I've just cloned this branch on my linux box. I'm running Python 3.6.6. It compiles fine, once I add "gfortran" to pgplot_libraries in setup.py. And it passes test_presto_python.py. So far, so good. I'll try to run some other tests.

@scottransom
Copy link
Owner Author

Yeah, all that stuff looks good. I'm worried mostly about a bunch of the *.py files in $PRESTO/bin. And also about many of the useful scripts in $PRESTO/python. Issues with integer division are what worry me the most.

@paulray
Copy link
Contributor

paulray commented Jul 11, 2018

Yeah, I see what you mean. I tried single_pulse_search and it doesn't work with either 2 or 3!

heselin0 : 793>python2 ./single_pulse_search.py -h
Traceback (most recent call last):
  File "./single_pulse_search.py", line 3, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

heselin0 : 794>python3 ./single_pulse_search.py -h
  File "./single_pulse_search.py", line 593
    ppgplot.pgmtxt('B', 2.5, 0.5, 0.5, "DM (pc cm\u-3\d)")
                                      ^
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 9-10: truncated \uXXXX escape

@paulray
Copy link
Contributor

paulray commented Jul 11, 2018

I see that you now have to pip install future, so that at least fixed the python2 issue.

Also, I think that probably there should now be a $PRESTO/lib/python2.7 and $PRESTO/lib/python3.6 so that compiled versions of the codes are separate. Is that a good idea?
Evidently, you can create presto.pth and put it in your site-packages directories and have it point to the correct place for each version of python.

Or, in the end the best solution may be to make it a python package so it gets installed in the appropriate site-packages directory and not to use PYTHONPATH, which is a bit of a hack.

m = -m
s = -s
self.dec = psr_utils.dms_to_rad(d, m, s)
if parts[part_index].split(":")[0]=="-00":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly recommend that parsing angle strings like this be done in astropy, so that dec=-0 bugs and the like don't creep in.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. That is old code that I have already flagged to convert. I'm going to update the way that the ATNF pulsar data are imported as well. Just haven't had time to do it yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured. I just like to pester you :-)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't blame you! I'd pester me too! That code is atrocious!!!

@paulray
Copy link
Contributor

paulray commented Jun 3, 2019

Any updates on when this might be merged? I'd like to switch to Python3 as my default python, but I don't want to lose psr_utils and all the PRESTO stuff

@scottransom
Copy link
Owner Author

scottransom commented Jun 3, 2019 via email

@paulray
Copy link
Contributor

paulray commented Jun 3, 2019

OK, so if I want to try that, and I have a clone of sransom/presto, what branch should I checkout?

@scottransom
Copy link
Owner Author

Hmmm. Well, it is probably easiest to install the python3new branch, since that is actually a branch. Although it isn't quite up-to-date with the pull request that I mentioned above. And now I notice that it has some conflicting files since I've since updated master. Maybe I can see if I can re-base it on the most recent master to help that a bit.

@scottransom
Copy link
Owner Author

OK. So I just fixed those merge conflicts. @paulray you should try to use this branch and see if you get any issues.

@paulray
Copy link
Contributor

paulray commented Jun 3, 2019

Thanks! I'm taking the plunge! I've made my default Python 3.6, with this PRESTO branch. We'll see how it goes...

@scottransom
Copy link
Owner Author

I know that some stuff will work. But I also know that there are almost certainly going to be a bunch of issues. Thanks for trying this!

@scottransom scottransom mentioned this pull request Nov 9, 2019
@scottransom
Copy link
Owner Author

Closing this because I finally merged #106

@scottransom scottransom deleted the python3new branch November 20, 2019 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants