Add documentation with DocFX

[J-M-White]

In response to #5, I followed the Bonsai Docfx Tutorial to add documentation that is consistent with other Bonsai packages. The information is the same as what was previously in the readme, with a plan to add more in future.

The website has not been published to Github Pages yet.

[banchan86]

hey @J-M-White just wanted to drop a note to say I will be reviewing this soon!

[banchan86]

Good job getting the docfx website up! I wrote some comments that could be addressed in my review. One general high level comment - as the bonsai workflow containers are not being used in this package (thus far) some related assets are redundant could be removed (such as the .bonsai environment, build.ps1 and etc). There are some areas where they could be useful (for instance, either the TriggerRecording or saving data from SpikeFetch/SpikeStream operators if that is possible).

[J-M-White]

Great! Thanks, @banchan86. I'll go through and address these comments.

[J-M-White]

Hi @banchan86, I have some updates for you!

In addition to directly addressing your comments, I've made the following changes:

• SpikeStream and SpikeFetch have been consolidated into a single operator, SpikeGLXInput. It replicates the behavior of one or the other depending on if it is subscribed to an input source. This is similar to how DigitalInput behaves in the DAQmx package. You were right that the distinction between SpikeStream and SpikeFetch was confusing - I think this is clearer.
• ImecChannelSelector has been marked as obsolete and its main feature (parsing string channel ranges into integer arrays) has been added to SpikeGLXInput.
• Articles have been added for each of the operators in the package.

Let me know what you think! In particular, I'm curious what you think should go in the Introduction/Readme - I expect that the authorship, acknowledgement, and license sections need to be updated.

[banchan86]

thanks @J-M-White I'll take a look at it soon!

[glopesdev]

@J-M-White thanks for working together to polish the package! We were also planning to do a repository reorganisation PR to align the project structure with the other repositories, but I guess since you started rethinking some of the operator names, I wanted to share a few thoughts on naming.

Package name

Technically SpikeGLX breaks the C# capitalisation rules for acronyms since GLX is a three-character acronym, but I feel in this case the distinctiveness of the suffix might justify it and I understand it was probably already published with this name before with this casing, so I would probably keep it and the namespace that goes with it unchanged.

Operator name

In general when integrating with existing APIs we try to keep terminology as close as possible to the original, so that it will be easier for people to relate to existing functionality. In this case it looks like the closest C# api name would be cs_sglx_fetch and I have noticed they use that term throughout their own documentation so it feels we probably would make the package easier to understand if we kept it.

If we called it SpikeGLXFetch it might work but again this would break the C# capitalisation rules for acronyms. In this case it is a bit more complex to parse as it is not the last word suffix, so I would really avoid it if at all possible. To match casing conventions we could call it SpikeGlxFetch or SglxFetch.

This last variant might be preferable as it would be very close to their own API and might even over time direct search matches to our own docs, which might be useful for people wanting to relate both. However, we would need to check how different the current operator works from their function.

It might be okay if it is slightly different, since Bonsai operators are much higher-level and can be expected to do initialisation work to setup a continuous observable stream, so if what the current operator is doing is mostly prepare the ground to call the Fetch operation either as a source, or triggered by an input, I would probably be happy with SglxFetch.

I have more points about structure, which I will raise in detail elsewhere but just wanted to mention the naming feedback since it came up, happy to hear your thoughts!

I can do a more close review and discussion on naming if helpful. In general it always pays off to do this at the outset since we want to minimise name changes later on to avoid breaking changes.

[J-M-White]

@glopesdev, thanks for that insight! Exactly like you said, what SpikeGLXInput is doing right now is laying groundwork and then calling cs_sglx_fetch repeatedly. Technically, when it is being triggered by an input it calls the API function cs_sglx_fetchLatest; however, that function itself calls cs_sglx_fetch. I think renaming SpikeGLXInput to SglxFetch would make a lot of sense.

In a similar vein, do you think TriggerRecording should be renamed to TriggerGT or SglxTriggerGT to align with the name of the underlying API function? DigitalOutput may be a more interesting case - it looks like the newest version of the API actually replaces the c_sglx_setDigitalOut function I've used with c_sglx_ni_DO_set for NI devices, and adds c_sglx_obx_AO_set for OneBoxes. Maybe it's worth renaming it now to something that explicitly indicates it's only for NI devices, so that there's no confusion if a separate operator is later added to interface with Oneboxes. What do you think?

Any review you have time to do (on naming or anything else) is much appreciated!

[banchan86]

Hi @J-M-White the documentation changes look great, especially with the creation of the individual operator articles and workflow examples. Just added a few comments that are docs specific if you want to address them for now. I can take another look after @glopesdev has updated the package repository and reviewed the naming/structure!

[glopesdev]

@J-M-White I think it might be easier if you do the naming changes before I do the repository restructuring.

It also might be useful to do the renaming / refactoring of operators in a separate PR, and then rebase this branch off that one, so that the name changes get listed clearly in the release notes.

I was revisiting the PR and thoughts on naming, and actually the easier name might be to use simply Fetch and Trigger. In the editor these will be listed as Fetch (SpikeGLX) and Trigger (SpikeGLX) since all operators are listed together with their namespace, and it might be less ambiguous that way.

[J-M-White]

@glopesdev I think Fetch and Trigger sound great! I've opened a new pull request (#7) with these name changes implemented. We can do any additional renaming/refactoring there and then rebase, like you said.

[glopesdev]

@J-M-White now that #7 has been merged, this branch will need to be rebased, but I was wondering maybe before finishing off the docs I could go ahead and restructure the repository to the common layout, which includes CI/CD and automatic release of packages to NuGet.

This should be quick now, and I have only one pending question, which is relating to the package icon. The standard layout includes the official Bonsai package icon as part of the standard repository structure, but I noticed you have a custom package icon image file included as part of the assets for this repo.

I was checking SpikeGLX and I couldn't find that it directly refers to their logo, so would you be opposed to changing to the default Bonsai package icon?

Let me know your thoughts and then I can quickly put together a PR with the restructuring so we can maybe finalize everything in the next couple of days.

[J-M-White]

@glopesdev, sounds good! I'm happy to change to the default Bonsai package icon. I think the current one is a holdover from before we started the process to make this an official package.

[CLAassistant]

All committers have signed the CLA.

[J-M-White]

@glopesdev & @banchan86, this branch has now been rebased off the restructured repo. I've also addressed (what I believe is) the last incomplete documentation suggestion: adding an example workflow for DigitalOutput. Let me know if I've missed anything!

[glopesdev]

This looks great, thank you so much for being so thorough!

I found it easier to add review comments to a separate PR to your branch (FeeLab#1), happy to discuss there or here.

[J-M-White]

The proposed changes in the separate PR all looked good to me! It's now been merged.

I'm glad to see that the official Bonsai policy is to use semicolons for separating long items in a list (I'm a big fan of this but have met with some resistance in the past).

[glopesdev]

@J-M-White thanks, I think we're almost there! Just noticed two files (logo.svg and favicon.ico) which are not needed anymore as they are included in the submodule. Sorry I didn't spot them in my earlier PR!

Other than that everything looks great and we should be ready to release. One last question I had is what version number to use for the release. Is it okay to use our usual starting point at 0.1.0? Or did you happen to use this one internally before?

P.S.: re. bullet list punctuation I am actually not too picky either way as long as there is consistency. In general we try to follow the MSDN Writing Style Guide for alignment with .NET documentation.

Interestingly, I just checked and they do recommend to avoid semicolons and conjunctions entirely in their Punctuation guide for lists. I'm used to seeing them in formal writing all the time, but there does seem to be mild consensus on avoiding them for web browsing as they are considered "too complex", so I'm torn here.

[J-M-White]

@glopesdev, great! Those two files have been removed.

In retrospect this was definitely not the smartest way to do this, but the internal versions I have are: 0.1.0-alpha, 0.1.1-alpha, 0.1.2-alpha, and 0.2.0-alpha.

I think I'm pro keeping the semicolons here, as long as you're fine with it.

[glopesdev]

the internal versions I have are: 0.1.0-alpha, 0.1.1-alpha, 0.1.2-alpha, and 0.2.0-alpha.

@J-M-White sounds good, is it okay then if we release this version as 0.2.0, or would you rather bump it to 0.3.0?

I think I'm pro keeping the semicolons here, as long as you're fine with it.

Yup, happy to do it; semicolons are the best punctuation mark after all ;)

[J-M-White]

@glopesdev, releasing as version 0.2.0 sounds good!

[glopesdev]

@J-M-White Initially spotted another unused file, so decided to do a full review and other things came up. All minor and should be quick to fix, I expect to merge after this round, thanks for your patience!

[J-M-White]

@glopesdev, no worries! I appreciate all your effort in getting the package ready for release! I've implemented the changes from most of the comments above. I just had a question about docs/api and the best way to structure the Trigger inputs (see above).