Add vectorization to 4 functions #317

michaelaye · 2019-11-02T18:44:09Z

using previously established pattern
fixes vectorize sct2e, scencd, and et2utc #310

Transferring the previously adapted style of vectorization from other functions

AndrewAnnex · 2019-11-02T19:08:50Z

I added a new function called isiterable to the support_types.py file that should be used instead when a iterable collection of strings is used, that should fix the failed tests...

…

-Andrew Annex

On Nov 2, 2019, at 2:44 PM, K.-Michael Aye ***@***.***> wrote: using previously established pattern fixes #310 You can view, comment on, or merge this pull request online at: #317 Commit Summary Add vectorization to several functions add tests for added vectorization File Changes M spiceypy/spiceypy.py (64) M spiceypy/tests/test_wrapper.py (42) Patch Links: https://github.com/AndrewAnnex/SpiceyPy/pull/317.patch https://github.com/AndrewAnnex/SpiceyPy/pull/317.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

AndrewAnnex

okay I have posted me review comments:

test are failing because you need to use the new if stypes.isiterable function for the 4 function instead of if hasattr
docstrings need to be updated to indication functions can return a collection
each function needs a separate unit test that just tests the vectorization independently
there are a lot of unnecessary white space diffs in the PR, those changes need to be removed.

spiceypy/spiceypy.py

AndrewAnnex · 2019-11-03T14:23:57Z

spiceypy/tests/test_wrapper.py

@@ -5701,6 +5703,8 @@ def test_scencd():
    sclkdp = spice.scencd(-32, sclkch)
    npt.assert_almost_equal(sclkdp, 985327950.0)
    assert sclkch == "2/20538:39:768"
+    mylist = 3 * [sclkch]
+    npt.assert_almost_equal(spice.scencd(-32, mylist), 3*[985327950.0])


for these and other new unit tests, they should be independent test functions that include vectorized in the function name ie test_scencd_vectorized. see rest of test_wrapper.py for examples.

AndrewAnnex · 2019-11-03T14:26:10Z

spiceypy/spiceypy.py

@@ -10985,8 +10993,16 @@ def scencd(sc, sclkch, MXPART=None):
    sc = ctypes.c_int(sc)
    sclkch = stypes.stringToCharP(sclkch)
    sclkdp = ctypes.c_double()
-    libspice.scencd_c(sc, sclkch, ctypes.byref(sclkdp))
-    return sclkdp.value
+    if hasattr(sclkch, "__iter__"):


for this function and for the other 4, change to use new iterable test function to fix failed builds

if stypes.isiterable(sclkch):

what about the other 14 uses of if hasattr?

I can fix all of them if you want, IF you let me remove those un-holy white-spaces at the end of lines and in empty lines as well? Deal? ;)

btw, i also found functions where you accept both scalar and iterable, but return type in the docstring does not mention it, e.g. srfxpt . Do those docstrings also need fixing?

another btw: as we want SpiceyPy to become PEP8 compliant in the future, right, we need to work on some stuff anyway. My editor removes them by default, that's why the white space was gone.

Quote from PEP8:

Avoid trailing whitespace anywhere. Because it's usually invisible, it can be confusing: e.g. a backslash followed by a space and a newline does not count as a line continuation marker. Some editors don't preserve it and many projects (like CPython itself) have pre-commit hooks that reject it.

i have updated all if hasattr to stypes.isiterable if they fit the pattern of looping over a potential iterable, otherwise do skalar type return. There were some cases with cells where that pattern didn't match, so i didn't touch those.

AndrewAnnex · 2019-11-20T17:06:31Z

@michaelaye do you have time to update this PR? If not could you enable edits from maintainers so I could wrap it up?

michaelaye · 2019-11-20T17:07:14Z

later today, yes. just finishing an MDAP proposal right now.

Improve readability

* Update spiceypy.py to use equality instead of is for small integer comparisons, not really a bug for cpython, but could be for other implementations

* removes old 3.X versions that are no longer supported in CI builds * updated docs section to include a link to the lessons * updated top warning to also say the naif writes tutorials that use spiceypy (the lessons), fixes AndrewAnnex#314

…Annex#315)

* start of numpy string support * added pandas test * noticed a missed error check in a loop, may need to fix in other places

@djcinsb

* added ev2lin, lightsail2 based unit test from @djcinsb * add coveralls to windows

not wrapped inside scalar version of test.

michaelaye · 2019-11-21T08:23:08Z

Seems like tests are still failing, ctypes wants a float not an iterable, it seems? How did you make it work for the other iterables?

>       et = ctypes.c_double(et)
456E       TypeError: a float is required

AndrewAnnex · 2019-11-21T16:17:13Z

README.rst

@@ -32,7 +32,7 @@ SPICE is an essential tool for scientists and engineers alike in the planetary
 science field for Solar System Geometry. Please visit the NAIF website for more details about SPICE.

 *IMPORTANT*: I have no current affiliation with NASA, NAIF, or JPL. The
-code is provided "as is", use at your own risk.
+code is provided "as is", use at your own risk. However, the NAIF now distributes python "lessons" that use SpiceyPy as the python to spice interface.


I have already added this to the master branch, but I am not sure why this diff is still being shown by github.

AndrewAnnex · 2019-11-21T16:18:01Z

README.rst

@@ -97,7 +100,7 @@ as newer versions are released.

 - OS: OS X, Linux, Windows
 - CPU: 64bit only!
- Python 2.7, 3.3, 3.4, 3.5, 3.6, 3.7
+- Python 2.7, 3.5, 3.6, 3.7


okay something strange is happening

AndrewAnnex · 2019-11-21T16:34:00Z

okay, so there a few things going on here. In the future please try to seperate these comments into different posts so I can reply to each individually.

I do not follow PEP8, or I mostly do? Either way, this PR must remain focused on getting these 4 functions working and up to spec with the rest of the codebase and should do nothing else. This policy is to keep the review process short and clear.
using hasattr is fine if the collection is a 1d list or array of numerical/boolean datatypes. stypes.isiterable should only be used for functions in which a list of strings are given at the moment I have not tested it for other use cases. If that function needs to be updated to be used uniformly across spiceypy let me worry about that in a later commit.
et2utc is failing because you are passing a collection of floats into ctypes.c_double, please look at the other vectorized functions that use hasattr to follow the existing pattern in the codebase.
Something is very wrong with sct2e now given the unit test errors I am seeing in the travis CI logs
utc2et is failing because you didn't delete the first call to stringToCharP
There is something really odd with the git history of this branch now, you pulled in a lot of diffs that I committed into master creating a very complicated diff that is hard to parse through. I think you can fix this by squashing your changes and rebasing onto the master branch then force pushing to overwrite this branch on your fork.
All white space diffs have to be reverted before I merge. I like to keep commits simple and although there might be good arguments to use black or pep8 in spiceypy, this PR is not the time for them.
Unit tests I suggested need to be added to test the vectorization of these 4 functions.

AndrewAnnex

unit test are failing, all white space diffs need to be removed, branch needs to be squashed then rebased onto the master branch to fix the git history and to clear up all of the bad diffs in one go

see below for long comment addressing these and other issues

okay, so there a few things going on here. In the future please try to separate these comments into different posts so I can reply to each individually.

1. I do not follow PEP8, or I mostly do? Either way, this PR must remain focused on getting these 4 functions working and up to spec with the rest of the codebase and should do nothing else. This policy is to keep the review process short and clear.

2. using hasattr is fine if the collection is a 1d list or array of numerical/boolean datatypes. `stypes.isiterable` should only be used for functions in which a list of strings are given at the moment I have not tested it for other use cases. If that function needs to be updated to be used uniformly across spiceypy let me worry about that in a later commit.

3. et2utc is failing because you are passing a collection of floats into ctypes.c_double, please look at the other vectorized functions that use hasattr to follow the existing pattern in the codebase.

4. Something is very wrong with sct2e now given the unit test errors I am seeing in the travis CI logs

5. utc2et is failing because you didn't delete the first call to stringToCharP

6. There is something really odd with the git history of this branch now, you pulled in a lot of diffs that I committed into master creating a very complicated diff that is hard to parse through. I think you can fix this by squashing your changes and rebasing onto the master branch then force pushing to overwrite this branch on your fork.

7. All white space diffs have to be reverted before I merge. I like to keep commits simple and although there might be good arguments to use black or pep8 in spiceypy, this PR is not the time for them.

8. Unit tests I suggested need to be added to test the vectorization of these 4 functions.

AndrewAnnex · 2019-11-21T16:24:45Z

spiceypy/spiceypy.py

    :return: Output epoch, ephemeris seconds past J2000.
-    :rtype: float
+    :rtype: Union[float,Iterable[float]]
    """
    utcstr = stypes.stringToCharP(utcstr)


delete this to fix the unit test

AndrewAnnex · 2019-11-21T16:34:40Z

spiceypy/tests/test_wrapper.py

@@ -5704,6 +5810,16 @@ def test_scencd():
    spice.kclear()


+def test_scencd_vectorized():
+    spice.kclear()
+    spice.furnsh(CoreKernel.testMetaKernel)


CoreKernels

AndrewAnnex · 2019-11-21T16:37:25Z

spiceypy/tests/test_wrapper.py

+    spice.furnsh(ExtraKernels.voyagerSclk)
+    et = spice.sct2e(-32, 985327965.0)
+    mylist = 3 * [985327965.0]
+    assert spice.sct2e(mylist) == 3*[et]


you are forgetting to pass in -32

michaelaye added 2 commits November 2, 2019 12:37

Add vectorization to several functions

a0b5408

Transferring the previously adapted style of vectorization from other functions

add tests for added vectorization

142393e

AndrewAnnex requested changes Nov 3, 2019

View reviewed changes

michaelaye and others added 8 commits November 21, 2019 01:06

Update README.rst (AndrewAnnex#312)

fa88920

Improve readability

Fix equality check for cells (AndrewAnnex#313)

dd93167

* Update spiceypy.py to use equality instead of is for small integer comparisons, not really a bug for cpython, but could be for other implementations

Update README.rst

b3a0f56

* removes old 3.X versions that are no longer supported in CI builds * updated docs section to include a link to the lessons * updated top warning to also say the naif writes tutorials that use spiceypy (the lessons), fixes AndrewAnnex#314

removing more is compairisons to be equality checks, not bugs (Andrew…

f0bff1a

…Annex#315)

numpy string support (AndrewAnnex#316)

4878ecb

* start of numpy string support * added pandas test * noticed a missed error check in a loop, may need to fix in other places

added ev2lin (AndrewAnnex#318)

bd632e3

* added ev2lin, lightsail2 based unit test from @djcinsb * add coveralls to windows

adding return type updates for iterables

564a9db

put iterable/vectorized tests as their own functions,

2a05936

not wrapped inside scalar version of test.

michaelaye requested a review from AndrewAnnex November 21, 2019 08:42

AndrewAnnex reviewed Nov 21, 2019

View reviewed changes

AndrewAnnex requested changes Nov 21, 2019

View reviewed changes

michaelaye closed this Nov 21, 2019

AndrewAnnex mentioned this pull request Jan 24, 2022

Allow np.ndarray inputs for subpnt, subslr, and pxform #435

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vectorization to 4 functions #317

Add vectorization to 4 functions #317

michaelaye commented Nov 2, 2019

AndrewAnnex commented Nov 2, 2019 via email

AndrewAnnex left a comment

AndrewAnnex Nov 3, 2019

AndrewAnnex Nov 3, 2019

michaelaye Nov 21, 2019

michaelaye Nov 21, 2019

michaelaye Nov 21, 2019

michaelaye Nov 21, 2019

michaelaye Nov 21, 2019

AndrewAnnex commented Nov 20, 2019

michaelaye commented Nov 20, 2019

michaelaye commented Nov 21, 2019

AndrewAnnex Nov 21, 2019

AndrewAnnex Nov 21, 2019

AndrewAnnex commented Nov 21, 2019

AndrewAnnex left a comment •

edited

Loading

AndrewAnnex Nov 21, 2019

AndrewAnnex Nov 21, 2019

AndrewAnnex Nov 21, 2019

Add vectorization to 4 functions #317

Add vectorization to 4 functions #317

Conversation

michaelaye commented Nov 2, 2019

AndrewAnnex commented Nov 2, 2019 via email

AndrewAnnex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewAnnex commented Nov 20, 2019

michaelaye commented Nov 20, 2019

michaelaye commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewAnnex commented Nov 21, 2019

AndrewAnnex left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewAnnex left a comment •

edited

Loading