Skip to content

Fix for calendar_tuple #103

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

Merged
merged 3 commits into from
Aug 27, 2016
Merged

Conversation

jrs65
Copy link
Contributor

@jrs65 jrs65 commented Aug 4, 2016

This pull request is a fix for #102.

I've also added some unit tests to check the conversion of calendar_tuple/julian_date, and wrapped the input of calendar_tuple in a _to_array call to stop it failing if called with a Python scalar argument.

@brandon-rhodes brandon-rhodes self-assigned this Aug 5, 2016
@brandon-rhodes
Copy link
Member

Thank you for noticing this problem and contributing a fix! I am getting an error when I try applying your fix to master, though:

  skyfield/tests/test_timelib.py line 234 in test_jd_calendar
    assert isinstance(cal[0], int)  # Year
AssertionError

This seems to result from the fact that cal[0] is of type:

<class 'numpy.int64'>

Could you add a print statement print(type(cal[0])) above the test line on your own machine, and let me know the result? I am curious whether, on your machine, it is coming out as a plain int, or whether you are using a better version of NumPy whose int64 type is also a subclass of Python’s native int.

Thanks for any extra information you can provide — I'd love to get this merged!

@jrs65
Copy link
Contributor Author

jrs65 commented Aug 9, 2016

When I insert that print statement I get <type 'numpy.int64'>. In both the versions of numpy that I have tried (Python 2.7.6, numpy 1.11.1; and Python 3.5.2, numpy 1.10.1), np.int64 is a subclass of int, i.e.

isinstance(np.int64(32), int)  # == True

similarly

isinstance(np.float64(17.0), float)  # == True

What version of numpy are you using? I can test with that specific version to see if I get the same results.

On a related note, is there a way to get assay to output the results of print statements? I actually had to pull out the test, so I could run it with plain python as it wasn't giving me any output.

@brandon-rhodes
Copy link
Member

If the test fails then stdout is printed — so adding a line like asdf at the end of the test is a quick way to get output. I guess I should add an option for not intercepting stdout / stderr.

@brandon-rhodes
Copy link
Member

brandon-rhodes commented Aug 25, 2016

Okay, I am back at my development machine so we can investigate further. I am using a slightly later version of NumPy with Python 3.5.1:

$ python
Python 3.5.1 |Continuum Analytics, Inc.| (default, Dec  7 2015, 11:16:01) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> np.__version__
'1.10.4'
>>> isinstance(np.int64(32), int)
False
>>> isinstance(np.float64(17.0), float)
True

I wonder if this is a known or open issue, or whether it’s not always guaranteed that NumPy types are subclasses of things like int and float?

@brandon-rhodes
Copy link
Member

Ah, perhaps some more information:

numpy/numpy#2951 (comment)

@jrs65
Copy link
Contributor Author

jrs65 commented Aug 25, 2016

Okay, great. I think I can just change the tests to be for numbers.Integral and numbers.Real.

@jrs65
Copy link
Contributor Author

jrs65 commented Aug 25, 2016

Alright. I've rebased and committed those changes. It works at my end, hopefully it works for you too!

@brandon-rhodes brandon-rhodes merged commit 10d7a89 into skyfielders:master Aug 27, 2016
@brandon-rhodes
Copy link
Member

Thank you for the great work! Your fix is absolutely correct, and your test is complete and easy to read.

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.

2 participants