Skip to content

Replace GValueArray with GstLALArray

Timothy Davies requested to merge tdavies__add_gstlal_array into spiir_compatibility_fixes

depends on !4 (merged)

Originally made as a patch in the SPIIR codebase, I've replicated that in this fork: lscsoft/spiir!59 (closed)

There are review comments there which I'll resolve before asking for review here.

Alternative

As Patrick Godwin noted in this issue, he has another (and I think much simpler) workaround: lscsoft/spiir#70 (comment 602061)
I think with a little extra work that workaround will probably beat this one out and will look cleaner from the Python & C side, though it would need to switch to GstValueArrays. That said, I'm inclined to keep trying this approach just in case the other one has issues. The ideal is that we get it working with Numpy arrays, but I don't know if that's possible.

Details

This MR defines a new type, GstLALArray, to set and get properties on gstreamer elements that previously used GValueArray.
It wraps a GArray with the simplest API I could come up with.

GstLALArray stores GValues, which in turn store a type and a value.
When appending, it checks the GValue's type to ensure it only stores one type.
To make a multidimensional array, you can append a GValue containing a GstLALArray.

Note that when returning a GValue from C to Python, it seems to be unwrapped. However, the other direction is less reliable. Some conversion is attempted from Python types to GValues, but as described in #70 it struggles with anything other than fundamental types. As such, the python code needs to assemble a GValue with the precise type and value when appending to a GstLALArray.

Note also that GI uses comment introspection to export the types from C. Most of the comments in gstlal_array.c, for example, are meaningful code that are used during the build process.

Shortcuts/Drafts (to be fixed before a serious review)

  • I've copy pasted some helper functions to convert GstLALArray to/from lists in python. If these helpers are acceptable, they should be in the GstLAL module.
  • The docs I've added aren't thoroughly considered
  • I couldn't get the namespaces in python (for GstLALArray and the preexisting GstLALFrHistory and GstLALGPSClock) to work quite right. The tag 'Method' was removed from a number of methods as part of that effort, and indeed it seems to be redundant, but maybe it should be verified.

The API is deliberately an MVP. It could definitely have more convenience features, but I think simple is reliable for now.

In general my emphasis was on getting something working so that testing could continue, but now that it's in that working state we'll need to consider the changes carefully.

Edited by Timothy Davies

Merge request reports