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

More python3 fixes #894

Merged
merged 2 commits into from
Aug 11, 2017
Merged

More python3 fixes #894

merged 2 commits into from
Aug 11, 2017

Conversation

timj
Copy link
Contributor

@timj timj commented Jul 21, 2017

This is the second batch of fixes required to build GalSim with a Python3 SCons.

I have successfully built GalSim using scons-3.0.0.alpha.20170614.tar.gz with Python3 but there is one issue I encountered with building of boost-python because those include files have bad Unicode in them for an author string. I've reported this upstream (and I have a local fix).

timj added 2 commits July 21, 2017 12:47
* Use get_text_contents() to ensure we get strings.
* Decode bytes from external commands.
* Do not open text files in binary mode.
execfile() does not work on python3
@timj
Copy link
Contributor Author

timj commented Jul 21, 2017

Everything did build and this patch only touches the scons files so I'm not quite sure why the tests failed.

@rmjarvis
Copy link
Member

It looks like the culprit is astropy 2.0. The default pip version for astropy switched from 1.3 to 2.0, and I'm guessing there must be something in there that is breaking some of the tests.

@jmeyers314 It's also happening with your PR. Could you take a look? The failures seem to be mostly with tests that you wrote, so you are probably best equipped to evaluate whether (and how) the test needs to change or if there is something in the backend code that needs to be fixed to accommodate their change. (My off-the-cuff guess is it's something with the astropy.units module.)

@timj In the meanwhile, if you want to change the pip install line in .travis.yaml to use astropy==1.3, that would probably make this PR pass the tests.

@jmeyers314
Copy link
Member

Looking at this now... Looks like at least part of the problem is that Planck's constant has changed! (maybe related to the upcoming redefinition of the kilogram?)... Working on a fix...

@rmjarvis
Copy link
Member

Yep. Looks like they switched from CODATA 2010 values to CODATA 2014.
image

@jmeyers314
Copy link
Member

I think this is fixed now on commit d4c602b if you want to cherry-pick that @timj. I also made a few other changes on PR #892 for things that were failing on my laptop, but not on Travis.

@cwwalter
Copy link
Member

@rmjarvis I'm curious: where did you find that plot? I found this:

http://www.codata.org/uploads/RMP.88.035009.pdf

But that has a somewhat different version.

@rmjarvis
Copy link
Member

@cwwalter
Copy link
Member

Thanks!

@rmjarvis rmjarvis merged commit 919b4e0 into GalSim-developers:master Aug 11, 2017
@timj timj deleted the u/timj/py3scons-2 branch August 11, 2017 17:48
@@ -12,7 +12,8 @@ try:
except:
# If SCons is using an old python (2.4 is relatively common still), then the above may fail.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I forgot to say was that the above code always fails so I'm not entirely sure what the purpose of the try block is. The comment is obviously out of date.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. The nada() call looks like maybe a way to force the except block for testing. git blame says it was me that added it, so probably I just forgot to remove it after testing that the except block was working properly. Oops. Did now. (Of course, the "2.4 is relatively common still" comment is indeed out of date. None of my systems have that anymore.)

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.

None yet

4 participants