Fix fatal error from deallocating Py_None #12

drbitboy · 2013-09-27T12:12:43Z

added Py_INCREF(Py_None) that was missing when returning Py_None

- Import traceback for debugging - Get base type of SpiceInt from typedef of gcc -E cspice/include/SpiceUsr.h - module globals spiceintType and spiceintType1 - Use envvar DEBUG_MKWRAPPER to set DEBUG - Added INOUTPUT_TYPE for variables that are both inputs and outputs - Parse IN/OUT/INOUT-PUT_TYPE from I, O, I/O, I-O, I,O in Brief_I/O section in document string of cspice/src/*_c.c - Added get_py_boolean for Py_True/_False input arguments - Added int for input argument (sigcode), with warning - Skip functions named *_ declared in gcc -E .../SpiceUsr.h - Skip functions names zz* declared in gcc -E .../SpiceUsr.h - Use local var funcnm for prototype_arg.function_name - Indent 2nd and laster lines comments with function definition - Put ioDict which contains IN/OUT/INOUT-PUT_TYPE info - Issue warning for void argument list in non-void argument list - Issue warning if input parameter cannot be parsed - Use I|O|I/O|I-O|I,O for setting IN/OUT/INOUT-PUT_TYPE - disable old code - disable old code about last_item - Add INOUTPUT_TYPE arguments to both input_list and output_list - Handle SpiceCell as pointer - Do not declare I/O argments - Move returnVal declaration to begining of routine - Make returnVal declaration for single return values other than found - Change indenting of output for py_to_c_conversions - Add .make_pointer to reasons for appending ouput name to t_list - Special handling as pointers for SpiceCell output arguments - Put {}s around PyObject* returnVal creation pyspice.c pyspice.h - Include malloc.h - Added get_py_boolean - Added gat/i/d/b to get time, int, double, bool from class instance attribute, specifically for spice.objects.Cell class instance - Parse spice.objects.Cell PyObject* to struct SpiceCell* - Added static data types to spice.objects.DataType class - Added type, size and length arguments to spice.objects.Cell.__init__ - Create spice.objects.Cell instance using these

pyspice.h - Include malloc.h - Added get_py_boolean - Added gat/i/d/b to get time, int, double, bool from class instance attribute, specifically for spice.objects.Cell class instance - Parse spice.objects.Cell PyObject* to struct SpiceCell* - Added static data types to spice.objects.DataType class - Added type, size and length arguments to spice.objects.Cell.__init__ - Create spice.objects.Cell instance using these

- added some comments - replaced malloc and free with PyMem_Malloc/_Free - return True from gen_wrapper instead of buffer.getvalue() - added count adjustment of prototypes for one-line prototypes pyspice.c pyspice.h - replaced malloc and free with PyMem_Malloc/_Free spice/objects.py - workable DataType and Cell classes

- changed logic of parsing prototypes, did not change result - change now includes one-line prototype in prototype counters - added some comments

mkwrapper.py - allow appnd_c calls (they work now!) - removed zz* calls - handle SpiceCell pointer outputs by making them required inputs also - added extra_inoutput_name_list - added extra_parse_tuple_string - moved py_to_c_conversions list before output handler loop - gen_wrapper returns True instead of full string pyspice.c - completed get_py_cell and get_spice_cell - N.B. get_spice_cell use PyMem_Malloc to create structure, - disabled SPICE_TIME and SPICE_BOOL cells spice/objects.py - bumped minimum character length to six because toolkit uses last char as terminator

- Key is variable name (e.g. 'c') - Value is tuple of toolkit routine names (e.g. 'whtevr_c') where that variable name should be an output but is labeled as an input in the -Brief_I/O sections of the source file

errdev_c call in initialization mkwrapper.py - use Py_BuildValue format of "s#" with output arg declaration "SpiceChar type [1]"

…tor frame angle; added comments; removed MYEARTH frame

…alculations; other mods.

michaelaye · 2013-09-27T18:45:45Z

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 notifications@github.com 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:

#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

rca · 2013-09-27T21:17:17Z

tests/test_horizons2plot.py

+    for kernel in self.kernels: spice.unload( kernel )
+
+  def test_horizons(self):
+    import horizons


The horizons module depends on connectivity to a JPL server. Can this test be updated so the connection is not necessary?

It's more of an example application, it also used matplotlib.

maybe it does not belong in the tests directory.

On Fri, Sep 27, 2013 at 5:17 PM, Roberto Aguilar
notifications@github.comwrote:

In tests/test_horizons2plot.py:

def setUp(self):

Load default kernels

mydir = os.path.dirname(file)

self.kernels = [ os.path.join( mydir,i.strip()) for i in

"""
+kernels/naif0010.tls
+kernels/spk_drm239_WithBurn-full.bsp

""".strip().split('\n') ]

for kernel in self.kernels: spice.furnsh( kernel )

def tearDown(self):

Unload any kernels

for kernel in self.kernels: spice.unload( kernel )

def test_horizons(self):

import horizons

The horizons module depends on connectivity to a JPL server. Can this test
be updated so the connection is not necessary?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12/files#r6639839
.

If it doesn't have the connection to JPL then we will need another SPK; it
could be small.

On Fri, Sep 27, 2013 at 8:18 PM, Brian Carcich drbitboy@gmail.com wrote:

It's more of an example application, it also used matplotlib.

maybe it does not belong in the tests directory.

On Fri, Sep 27, 2013 at 5:17 PM, Roberto Aguilar <notifications@github.com

wrote:

...

The horizons module depends on connectivity to a JPL server. Can this
test be updated so the connection is not necessary?

Ah, okay. I think samples are great actually. Maybe make a samples directory at the project root. Unless the SPK is massive I think having some data files in the repo makes sense to provide sample usage.

rca · 2013-09-27T21:19:03Z

@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.

michaelaye · 2013-10-15T04:42:01Z

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?

drbitboy · 2013-10-15T18:40:01Z

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
notifications@github.comwrote:

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//pull/12#issuecomment-26308975
.

drbitboy · 2013-10-15T18:59:15Z

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 drbitboy@gmail.com 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 <notifications@github.com

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//pull/12#issuecomment-26308975
.

drbitboy · 2013-10-15T19:17:49Z

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 drbitboy@gmail.com 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 drbitboy@gmail.com 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 <
notifications@github.com> 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//pull/12#issuecomment-26308975
.

michaelaye · 2013-10-15T23:41:57Z

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.

rca · 2013-10-16T16:50:56Z

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.

michaelaye · 2013-10-16T18:31:47Z

how about auto-download of required files?

rca · 2013-10-16T23:10:03Z

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//pull/12#issuecomment-26444680
.

michaelaye · 2013-10-16T23:10:53Z

Isn't the SPICE NAIF server world-wide reachable?

drbitboy · 2013-10-17T01:15:08Z

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 notifications@github.comwrote:

Isn't the SPICE NAIF server world-wide reachable?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-26467102
.

rca · 2013-10-17T04:46:43Z

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?

drbitboy · 2013-10-17T05:07:49Z

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
notifications@github.comwrote:

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//pull/12#issuecomment-26479044
.

michaelaye · 2013-10-22T19:18:44Z

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.

mkwrapper.py Special code to get gcpool working; should work for others setup.py My build stopped working (why?), this fixed it

Solves a KeyError in recent versions of Mac OSX

Update getnaifspicetoolkit.py

include and library files are handled in module1 below

srijit2021 · 2021-08-23T07:48:49Z

PyObject*
some_method(PyObject *self, PyObject *args)
{
[...snip...]

/* Fixed version of the above: */
Py_RETURN_NONE;

}

michaelaye · 2021-08-23T18:25:54Z

this library is not being maintained anymore, you should use SpiceyPy instead: https://github.com/AndrewAnnex/SpiceyPy

drbitboy added 19 commits January 17, 2013 00:59

Merge remote-tracking branch 'upstream/master'

8fdbce1

mkwrapper.py

20330b2

- changed logic of parsing prototypes, did not change result - change now includes one-line prototype in prototype counters - added some comments

Fix for output arguments mislabled as inputs in source files

b332744

- Key is variable name (e.g. 'c') - Value is tuple of toolkit routine names (e.g. 'whtevr_c') where that variable name should be an output but is labeled as an input in the -Brief_I/O sections of the source file

make correct outputs for fixed-length character arrays, and disabled

52da4eb

errdev_c call in initialization mkwrapper.py - use Py_BuildValue format of "s#" with output arg declaration "SpiceChar type [1]"

added some tests

1f96d3b

Added comments, rearranged some lines, no functional change

ad67b53

make correct outputs for fixed-length character arrays, and disabled

64d34da

errdev_c call in initialization mkwrapper.py - use Py_BuildValue format of "s#" with output arg declaration "SpiceChar type [1]"

Moved matplotlib code outside unittest purview

10f47ff

Added SMAP test case

903a411

Added comments about SMAP kernel; at 509MB it is not included in repo

03f4fee

added Earth/Sun orbit frame to NAIF_BODY{NAME,CODE}; corrected reflec…

ce2a00e

…tor frame angle; added comments; removed MYEARTH frame

Generated test SMAP SPK; updated SMAP test; removed orbital element c…

af12b45

…alculations; other mods.

Type 05 SPK and code to write it for SMAP test

c516f4a

added some files to .gitignore; misc cleanup

a9a9b11

added Py_INCREF(Py_None) that was missing

6bb2cd8

rca reviewed Sep 27, 2013
View reviewed changes

drbitboy and others added 7 commits November 2, 2013 10:52

Added correction for mout In/Out error in CSPICE comments

ceb9d08

Added another correction for mout In/Out error in CSPICE comments

00b921a

get gcpool working; slight mod to setup.py

086228d

mkwrapper.py Special code to get gcpool working; should work for others setup.py My build stopped working (why?), this fixed it

temporary approach for handling SPICE toolkit N0065

e044697

Update getnaifspicetoolkit.py

89a582b

Solves a KeyError in recent versions of Mac OSX

Merge pull request #1 from matteobachetti/patch-1

a9012bb

Update getnaifspicetoolkit.py

removed unused include and library setup

4fc769f

include and library files are handled in module1 below

Fix fatal error from deallocating Py_None #12

Are you sure you want to change the base?

Fix fatal error from deallocating Py_None #12

Uh oh!

Conversation

drbitboy commented Sep 27, 2013

Uh oh!

michaelaye commented Sep 27, 2013

Uh oh!

rca Sep 27, 2013

Choose a reason for hiding this comment

Uh oh!

drbitboy Sep 28, 2013

Choose a reason for hiding this comment

Load default kernels

Unload any kernels

Uh oh!

drbitboy Sep 28, 2013

Choose a reason for hiding this comment

Uh oh!

rca Sep 28, 2013

Choose a reason for hiding this comment

Uh oh!

rca commented Sep 27, 2013

Uh oh!

michaelaye commented Oct 15, 2013

Uh oh!

drbitboy commented Oct 15, 2013

Uh oh!

drbitboy commented Oct 15, 2013

Uh oh!

drbitboy commented Oct 15, 2013

Uh oh!

michaelaye commented Oct 15, 2013

Uh oh!

rca commented Oct 16, 2013

Uh oh!

michaelaye commented Oct 16, 2013

Uh oh!

rca commented Oct 16, 2013

Uh oh!

michaelaye commented Oct 16, 2013

Uh oh!

drbitboy commented Oct 17, 2013

Uh oh!

rca commented Oct 17, 2013

Uh oh!

drbitboy commented Oct 17, 2013

Uh oh!

michaelaye commented Oct 22, 2013

Uh oh!

srijit2021 commented Aug 23, 2021

Uh oh!

michaelaye commented Aug 23, 2021

Uh oh!

Uh oh!