Metadata handling, export, and real-time visualization

[t-sasatani]

Some updates for interfacing metadata and accompanying updates for the CLI.

Changes:

• Patch error _buffer_to_frame #34
• Add metadata CSV export and circular buffer queue shared list (for use in other processes)
• Add BufferedCSVWriter for buffered CSV exports.
• Change CLI to output files with the same stem names (for -o sample, you get sample.avi, sample.csv, sample.bin)

Notes:

• I used the runtime config module to add some environment variables, but I am unsure if I used it as @sneakers-the-rat intended. I would appreciate feedback.
• Now, the video export is locked to .avi, which is good for now, but it might be better to define this by environment variables or something. There's no point doing this now, so we can probably leave it until we build an interface for video export options.

[coveralls]

coverage: 73.757% (+1.7%) from 72.069%
when pulling 479363c on feat-metadata-handle
into 2290874 on main.

[sneakers-the-rat]

Ope sorry missed this when u opened it. I got some notes but like what yr thinkin here :)

[t-sasatani]

No problem. This isn't a hurry at all!

[sneakers-the-rat]

ok @t-sasatani see my updates in the above commits, tried to split them up so they are a little easier to follow, but they are sorta intrinsically related to each other.

Basically i am trying to have as light of a touch as possible here bc i know y'all are under a deadline while trying to avoid this package getting too spaghettilike from that rush against deadline.

• Move streamdaq runtime config to its own configuration model: since these are parameters that should be generally true for this device, moved them there - the top-level Config object is for behavior that should be true across the entire package. as this package grows we want to avoid putting things at the top level if they are only used in some part of the module. We also want to avoid having too many ways to configure things without a clear structure in place: streamdaq config is already set in those yaml files, so kept it there.
• Move csvwriter out of the inner loops which are perf sensitive to the outer loop which shouldn't be. We'll need a bigger refactor here eventually, but as i said above trying to not touch too much while we're on deadline. those inner methods should be super minimal and only do exactly one thing. the capture method is already a junk drawer method (hence need for refactor) but just trying to keep similar things in similar places for now.
• Make plotter handle its own timing - simplify the API to that class so it's just "give me objects to plot" instead of every object that uses it needing to manage draw timing. I think you might also like some of the other tidying there, hopefully that makes sense.
• Make basic e2e tests for these functions - we'll need a great deal more testing for this whole thing, the entirety of the inner methods are untested atm which i suspect is the source of continual regressions, but at least we can keep up with top-level tests.

There are some test errors that are only showing up in CI on ubuntu that i'm gonna fix, but other than that i'll take one final look and approve

[sneakers-the-rat]

ok here's a fun one for you - for the csv writer test, where since i didn't know exactly what we would want there, i just did a simple shape test, where the header metadata that gets written should be the same every time (for a given test data file), but we are getting...

• linux: 893 rows
• windows: 901 rows
• mac: 901 rows
  (and, locally, on my machine, getting 855 rows, which is why the value is set that way).

I'm not really sure what that value should be since idk the source data, but the video is 20fps, 5.5s long = 110 frames, and there should be 8 buffers per frame = 880 total, which is actually none of those values.

so... ya...

[sneakers-the-rat]

OK fixed those bugs, and fixed the early termination bugs - removed ability to exit with escape while displaying videos, now there are two ways to quit a capture:

• terminate event is the underlying holder of quitting logic
• ctrl+c causes terminate to get flipped to true.

but instead of prior behavior where all the loops quit immediately when getting the terminate event, refactored the top-level capture frame processing logic into a separate method so it was possible to call in two places, so now we explicitly wait for the queues to drain before fully quitting (it's also possible to force quit with a second ctrl+c).

very very strong need to break apart those inner methods so that we have better access to them for unit testing, bc we are gonna keep getting mystery errors as long as we only have end-to-end tests and can't directly test them as pure functions, given some expected input, get some expected output, separated from the rest of the acquisition process.

[sneakers-the-rat]

not sure when this got switched to just being a string that looks like a list of strings instead of being an actual list of strings, but this makes the test considerably weaker.

in particular, str(x) in str(y) checks if that substring is anywhere in the other string, but str(x) in list(y) checks whether exactly str(x) is found in the list of strings. So eg.

>>> "" in '["some other string", "that shouldn\'t include", "the empty string literal"]'
True
>>> "" in ["some other string", "that shouldn\'t include", "the empty string literal"]
False

[sneakers-the-rat]

Added tests that confirm that varying buffer size doesn't change output to guard against the problems @t-sasatani was describing here: #35 (comment) which was because of the termination logic

[sneakers-the-rat]

Down with merging this btw :)

[t-sasatani]

Thanks sounds great. I'm thinking about making some changes to save more messed-up metadata, but I will bump up a minor version after I decide what to do with that.

[sneakers-the-rat]

Merge this then do that?

[t-sasatani]

I took a look, and never mind about the metadata stuff: I thought the plotted and saved metadata might differ, but it was because I was looking at them on different scales. I just have one question in this review, which can also be a quick patch in the next PR.

[t-sasatani]

Is there a reason why you want to couple these two tightly? I would instead like to have these in separate queues and separate for loops (pasted below) because we want to know metadata more when we don't get images.

for image, header_list in exact_iter(imagearray.get, None):

[t-sasatani]

pasted below -> pasted the corresponding line in the current code
because I don't know if referring multiple code blocks in one comment is possible.

[t-sasatani]

I guess coupling these might be better for unifying the data flow. Just added a line to rescue some metadata even when an image can't be recovered.

[t-sasatani]

The refactored version is running very slow compared to 5d33e3b, and I'm looking why. I assume it's blocking/non-blocking stuff in plots, and here is an example binary to see this.
https://drive.google.com/open?id=1QoLB1F7HUau6TDrHQWxpivR48RhpTfWN&usp=drive_fs

[t-sasatani]

This was just the window refresh (cv2.waitKey(1)) getting deleted with the ESC key escape. Also, the plot window becomes unstable when moved (changing the GUI backends of matplotlib didn't fix it), but I guess I'll merge it with an experimental notice.

It might be worth considering GUI tests, but I'm unsure if there's a good way to do non-web GUI tests on GitHub actions.