PySPICE
PySPICE copied to clipboard
rca
Fix fatal error from deallocating Py_None
Awesome! Thanks so much. I am currently down with a cold and can't focus very much, but as soon as I got a minute I will try out your new stuff. Especially the addition of cells makes me very happy as many people are using those these days.
Michael
Sent from my iPad
On Sep 27, 2013, at 5:12 AM, Brian Carcich [email protected] wrote:
added Py_INCREF(Py_None) that was missing when returning Py_None
You can merge this Pull Request by running
git pull https://github.com/drbitboy/PySPICE master Or view, comment on, or merge it at:
https://github.com/rca/PySPICE/pull/12
Commit Summary
mkwrapper.py pyspice.c Merge remote-tracking branch 'upstream/master' mkwrapper.py mkwrapper.py Functioning SpiceCells! Fix for output arguments mislabled as inputs in source files make correct outputs for fixed-length character arrays, and disabled added some tests Added comments, rearranged some lines, no functional change make correct outputs for fixed-length character arrays, and disabled Moved matplotlib code outside unittest purview Added SMAP test case Added comments about SMAP kernel; at 509MB it is not included in repo added Earth/Sun orbit frame to NAIF_BODY{NAME,CODE}; corrected reflector frame angle; added comments; removed MYEARTH frame Generated test SMAP SPK; updated SMAP test; removed orbital element calculations; other mods. Type 05 SPK and code to write it for SMAP test added some files to .gitignore; misc cleanup added Py_INCREF(Py_None) that was missing File Changes
M .gitignore (5) M mkwrapper.py (421) M pyspice.c (271) M pyspice.h (9) M spice/objects.py (48) A tests/horizons.py (114) A tests/kernels/makespk05.c (61) A tests/kernels/naif0010.tls (144) A tests/kernels/pck00009.tpc (3639) A tests/kernels/smap_test.bsp (0) A tests/kernels/smap_v00.tf (157) A tests/kernels/spk_drm239_WithBurn-full.bsp (0) A tests/test_btc.py (118) A tests/test_horizons2plot.py (110) A tests/test_ison.py (259) A tests/test_smap.py (102) Patch Links:
https://github.com/rca/PySPICE/pull/12.patch https://github.com/rca/PySPICE/pull/12.diff
@drbitboy thanks for the patch; especially adding tests, they are much needed!
I'll leave it to @michaelaye to verify the change when he is well.
Is there a way to have getfov_c included now? I hacked it in my fork with some hardcoded cheating (see my fork), but do we now have a clean way to add it?
Without looking, i assume the issue is the length input arguments of output strings or other arrays, which is what is requiring us to exclude several routines. I think the way to handle that semi-generically is to specify some length/varname pairings, by routine name (e.g. similar to how we do the excluded routines now and some other things). It will be a big task to go through and find all the routines the first time, but after that only tweaks will be required if NAIF adds new routines.
That way it is data-driven, stored in one place, and easily fixed if the interfaces change (unlikely except for new routines). Also, I think Roberto's original approach (looking for a variable "lenout" and applying it to the first output pointer, or just using a constant length for strings, IIRC) that eliminates the input argument(s) that specify output length(s) should be dropped and we should change the PySPICE interface to require the user to enter the maximum length(s) for strings and arrays. It's a little messy as we will have to malloc space, and then free it for all return paths. I know I almost always use 99 for string values in ICY (the IDL/SPICE interface). It is not much of an inconvenience and easily pays for itself by duplicating the CSPICE interfaces so you don't have to think about which arguments are used or not.
On Tue, Oct 15, 2013 at 12:42 AM, K.-Michael Aye [email protected]:
Is there a way to have getfov_c included now? I hacked it in my fork with some hardcoded cheating (see my fork), but do we now have a clean way to add it?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26308975 .
yeah, I looked at getfov_c; it's a real gem but a perfect example.
So a global dict something like this:
lengthDict = dict( getfov=[ ["shapelen", "shape"], "framelen frame".split(), "room bounds".split() ], ...)
(ignore my ["...","..."] vs. "... ...".split() nonsense - I find the .split() form cleaner, quicker to type and easier to fix; in practice it will be some parsed lines of a triple-quoted string)
or even
lengthDict = dict( getfov=dict( shape=True, frame=True, bounds=["room",3] ), ... )
where var=True is shorthand for var="varlen"
Also, bounds=["room",3] could be "room*3" (for eval()), or even just bounds="room" since the [3] is parsed elsewhere.
that plus some more code to check and process that dict should do it:
On Tue, Oct 15, 2013 at 2:40 PM, Brian Carcich [email protected] wrote:
Without looking, i assume the issue is the length input arguments of output strings or other arrays, which is what is requiring us to exclude several routines. I think the way to handle that semi-generically is to specify some length/varname pairings, by routine name (e.g. similar to how we do the excluded routines now and some other things). It will be a big task to go through and find all the routines the first time, but after that only tweaks will be required if NAIF adds new routines.
That way it is data-driven, stored in one place, and easily fixed if the interfaces change (unlikely except for new routines). Also, I think Roberto's original approach (looking for a variable "lenout" and applying it to the first output pointer, or just using a constant length for strings, IIRC) that eliminates the input argument(s) that specify output length(s) should be dropped and we should change the PySPICE interface to require the user to enter the maximum length(s) for strings and arrays. It's a little messy as we will have to malloc space, and then free it for all return paths. I know I almost always use 99 for string values in ICY (the IDL/SPICE interface). It is not much of an inconvenience and easily pays for itself by duplicating the CSPICE interfaces so you don't have to think about which arguments are used or not.
On Tue, Oct 15, 2013 at 12:42 AM, K.-Michael Aye <[email protected]
wrote:
Is there a way to have getfov_c included now? I hacked it in my fork with some hardcoded cheating (see my fork), but do we now have a clean way to add it?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26308975 .
Only 56 likely SpiceChar outputs:
% grep 'SpiceChar [] [a-zA-Z0-9]_ _[,)]' ~/cspice/src/cspice/__c.c| grep -vE 'malloc|ConstSpiceChar|return' | wc 56 266 4950
so may 100 or so total.
-b
On Tue, Oct 15, 2013 at 2:59 PM, Brian Carcich [email protected] wrote:
yeah, I looked at getfov_c; it's a real gem but a perfect example.
So a global dict something like this:
lengthDict = dict( getfov=[ ["shapelen", "shape"], "framelen frame".split(), "room bounds".split() ], ...)
(ignore my ["...","..."] vs. "... ...".split() nonsense - I find the .split() form cleaner, quicker to type and easier to fix; in practice it will be some parsed lines of a triple-quoted string)
or even
lengthDict = dict( getfov=dict( shape=True, frame=True, bounds=["room",3] ), ... )
where var=True is shorthand for var="varlen"
Also, bounds=["room",3] could be "room*3" (for eval()), or even just bounds="room" since the [3] is parsed elsewhere.
that plus some more code to check and process that dict should do it:
On Tue, Oct 15, 2013 at 2:40 PM, Brian Carcich [email protected] wrote:
Without looking, i assume the issue is the length input arguments of output strings or other arrays, which is what is requiring us to exclude several routines. I think the way to handle that semi-generically is to specify some length/varname pairings, by routine name (e.g. similar to how we do the excluded routines now and some other things). It will be a big task to go through and find all the routines the first time, but after that only tweaks will be required if NAIF adds new routines.
That way it is data-driven, stored in one place, and easily fixed if the interfaces change (unlikely except for new routines). Also, I think Roberto's original approach (looking for a variable "lenout" and applying it to the first output pointer, or just using a constant length for strings, IIRC) that eliminates the input argument(s) that specify output length(s) should be dropped and we should change the PySPICE interface to require the user to enter the maximum length(s) for strings and arrays. It's a little messy as we will have to malloc space, and then free it for all return paths. I know I almost always use 99 for string values in ICY (the IDL/SPICE interface). It is not much of an inconvenience and easily pays for itself by duplicating the CSPICE interfaces so you don't have to think about which arguments are used or not.
On Tue, Oct 15, 2013 at 12:42 AM, K.-Michael Aye < [email protected]> wrote:
Is there a way to have getfov_c included now? I hacked it in my fork with some hardcoded cheating (see my fork), but do we now have a clean way to add it?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26308975 .
Ok, we need to decide how to proceed with the tests, guys. @drbitboy Brian obviously did a hell of a job to improve a lot of things, but
- it's so much, I don't understand yet, which parts are working differently and/or better now? ** E.g. is the added parsing of the doc-text required to implement cells?
- because I cannot run any tests due to missing kernels I can't verify/validate much apart with my own software, that is not using cells yet. Are there any tests for cells?
Can we either have a list of the kernels that need to be there or to change the tests in a way that they can run off-line?
Another question, to Brian, should we do these changes before merging this pull request, or would you prefer not to spend so much time on it, merge it to an extra branch, improve things there and then merge it to master later?
Let me know what you guys think.
With regards to tests, anybody with access to the repository (the world) should be able to run tests. The test suite should be self-contained such that everything needed to run the tests is in the repository. Of course, with the exception of any third party libraries necessary, for example, Python's mock library, which instead should be listed in the project's requirements.txt file so it can be easily pip installed.
As in one of my previous comments, one of the tests connected directly to a JPL server. That test should be refactored to eliminate this requirement so that folks outside the lab can run the tests too.
Test data files should also be included, but everyone should be mindful not to bloat the repository with massive data files just to run simple tests. i.e. kernels needed to run the tests should be in the repository.
Hope this helps.
The question then becomes from where?
On Wednesday, October 16, 2013, K.-Michael Aye wrote:
how about auto-download of required files?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26444680 .
yes, and so is horizons, which is where the other test data come from
On Wed, Oct 16, 2013 at 7:10 PM, K.-Michael Aye [email protected]:
Isn't the SPICE NAIF server world-wide reachable?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26467102 .
What's the retention policy on kernel files on those systems? If the files ever go away the tests will break.
This is a healthy discussion and I'm glad we're having it. Can you explain the reluctance towards keeping test files in the repo?
reluctance on size.
i've rarely seen a file disappear, although daily CKs and SPKs will be merged once each year or so, and old versions of kernels will be put into zzarchive/ or a_old_versions/ subdirs.
but as long as we choose carefully (e.g. ftp://naif.jpl.nasa.gov/pub/naif/generic_kernels/, hmm, they may be part of CSPICE; or use one of the a_old_versions/ subdirs) we should be okay.
-b
On Thu, Oct 17, 2013 at 12:46 AM, Roberto Aguilar [email protected]:
What's the retention policy on kernel files on those systems? If the files ever go away the tests will break.
This is a healthy discussion and I'm glad we're having it. Can you explain the reluctance towards keeping test files in the repo?
— Reply to this email directly or view it on GitHubhttps://github.com/rca/PySPICE/pull/12#issuecomment-26479044 .
Actually I totally love your hackorz trick of doing 'abc def'.split() instead of typing ['abc','def']. I am using it already everywhere! ;)
I am a bit under water with finishing the MDAP proposal, continue here later.
PyObject* some_method(PyObject *self, PyObject *args) { [...snip...]
/* Fixed version of the above: */
Py_RETURN_NONE;
}
this library is not being maintained anymore, you should use SpiceyPy instead: https://github.com/AndrewAnnex/SpiceyPy