Restructure classes and workflow

[stephprince]

Based on our discussion earlier this week, we have decided to reorganize responsibilities among the main aqnwb classes.

These changes

• remove the NWBRecording class, the user will instead have control of the NWBFile object and the openFile/closeFile processes
• give the user control over I/O object creation to pass into the NWBFile
• move RecordingContainers to its own file, it will manage access / writing to datasets
• remove startRecording and stopRecording from the NWBFile, leave that responsibility to the I/O

I've also added documentation describing the workflow we discussed.

[stephprince]

@oruebel I just noticed that you had started outlining the workflow in the other PR as well - we can try to merge them here or keep the documentation in a separate PR?

[oruebel]

@oruebel I just noticed that you had started outlining the workflow in the other PR as well - we can try to merge them here or keep the documentation in a separate PR?

No worries about that. #80 is just a draft PR. I just wanted to keep notes about the workflow and I figured it was easiest to just put it in a draft PR so we have something to build off rather than bury the notes in a GoogleDoc. We can shepherd this PR through first. If #80 still has something useful to add after we are done with this PR, then I can work on that or otherwise we can just close #80.

[oruebel]

Overall, this looks good to me. I add some comments on the workflow.dox, mainly to: 1) add a bit more detail about what the role of different classes are to clarify why they are needed and 2) change class names to references to make it easier for readers to find more information. The other high-level item is that I think RecordingContainers should accept all Container types not just TimeSeries, we'll likely need non-TimeSeries for recording in the near future so I think it would be good to not restrict this.

[oruebel]

I closed #80 since you've covered everything that was in that PR here.

[oruebel]

Fix #48 Once this PR is merged we have I believe addressed everything that is listed in that issue

[oruebel]

By using std::move and using std::unique_ptr<Container> data we require that ownership is being transferred from the caller to to the containers vector. I believe this means two things:

1. the caller cannot just call addData with a Container pointer must use std::move
2. the caller loses ownership of the Container and cannot access the data anymore

E.g.:

RecordingContainers rc;
std::unique_ptr<Container> myContainer = std::make_unique<MyContainer>(1);
rc.addData(std::move(myContainer));  // Must use std::move here
// myContainer is a nullptr now

If we are saying that RecordingContainers is taking on ownership of the Container, then I think this makes sense. Does NWBFile need to keep the Container it creates around or does it give up ownership. I believe it is the former (in which case this is fine), but I just wanted to confirm.

However, this transfer of ownership is a detail that may not be obvious to users, so it would be useful to document this side-effect in the docstring of the function for developers and if a user is intended to call addData` directly (rather than, e.g.,`NWBFile.createElectricalSeries` calling it on the users behalf), then we should also mention it in the user workflow docs. However, if it is just happening behind the scences, then mentioning it in the docsting of addData`` should be sufficient.

[oruebel]

How does the user know what the index of the ElectricalSeries is? I.e., how does the user know how to access the ElectricalSeries? Is this currently implicit, i.e., the user needs to remember the order in which they added objects to the recordingContainers object?

If so, I'd suggest to either: 1) change createElecticalSeries to return the index of the object that was added, or 2) add a line here along the lines of SizeType electricalSeriesIndex = recordingContainers.get().size() - 1 along with some text in the workflow docs to indicate that : a) new objects are always appended at the end in RecordingContainers and b) that this is how we can get and remember the index needed to retrieve the object later on. Either option seems fine, but it will be useful to document how we can get the index we should use to access a particular Container that was added to RecordingContainers

[stephprince]

I added containerIndexes as an optional input that will let users keep track of which index has been created by createElectricalSeries.

I think this approach will keep it more consistent that functions that might modify the NWBFile like createElectricalSeries return their status (e.g. returning a failure if the io->canModifyObjects() is false).

createElectricalSeries can also add multiple containers (depending on the size of recordingArrays) to the recordingContainers object so I think having it within the function might also be easier for the user rather than keeping track of the size before and after createElectricalSeries is called.

[oruebel]

I added a couple of minor changes in workflow.dox (mainly to fix line breaks from my previous comments) and added a couple of minor comments to enhance documentation of a few details that may not be obvious to a user. Otherwise this looks good.