NeurodataWithoutBorders / matnwb

A Matlab interface for reading and writing NWB files
BSD 2-Clause "Simplified" License
49 stars 32 forks source link

add new raw/scratch script #534

Closed lawrence-mbf closed 10 months ago

lawrence-mbf commented 10 months ago

Motivation

Adds Raw/Scratch tutorial from this pynwb guide

How to test the behavior?

Documentation should be correct and cover the same topics

Checklist

codecov[bot] commented 10 months ago

Codecov Report

Merging #534 (e4fd45a) into master (286d6b5) will increase coverage by 0.00%. Report is 5 commits behind head on master. The diff coverage is 96.15%.

@@           Coverage Diff           @@
##           master     #534   +/-   ##
=======================================
  Coverage   87.71%   87.72%           
=======================================
  Files         132      132           
  Lines        5584     5620   +36     
=======================================
+ Hits         4898     4930   +32     
- Misses        686      690    +4     
Files Changed Coverage Δ
+types/+util/checkDtype.m 87.93% <96.15%> (+1.26%) :arrow_up:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

bendichter commented 10 months ago

@lawrence-mbf thanks for putting this together!

  1. I changed the style from
func( ...
    'arg', arg ...
   , 'arg2', arg2');

to

func( ...
    'arg', arg, ...
    'arg2', arg2');

For the tutorials, I am adopting a style that is an approximate translation from Python's Black. I appreciate that your approach provides the benefit that removing a line only changes that one line, and accounts for the fact that a trailing is allowed in Python but not in MATLAB. However, I have never seen code in this style and I think it might be confusing. Are you following an established style standard here? Otherwise, I'd prefer to bite the bullet and put the commas on the line of the arg, even though I appreciate the point that we are losing the benefit of having line changes be completely self-contained.

  1. Could you add a screenshot of HDFView showing that the resulting file does indeed use an external link?

  2. Is it necessary to have the scratch demo also use external links? It seems like this is combining two concepts that do not need to be combined and making the tutorial more complex than it needs to be. If a user is interested in using the scratch space, I'd prefer to have a tutorial that does not require them to understand external links first. Maybe it would make sense to separate these into completely different mlx files?

  3. Could you please add a link from the README to the html files similar to we have for the other tutorials?

lawrence-mbf commented 10 months ago

@bendichter

(1) Regarding multiline function style

AFAIK there is no coding standard in MATLAB other than auto-indentation (CTRL+I). This adoption is purely from modifying MATLAB's name-value args. I'm not sure why adopting a python-specific style would help MATLAB style but I can change that for the script anyway.

(3) External Links

This was pulled from the documentation here and I'm trying to follow the major topic as closely as I can. It is true that external linkage is a viable solution for data analysis so I wanted to make that clear that you can do that too (albeit, manually).

Note that the pynwb documentation does refer to External Links but refers to the feature (confusingly) as "Copying" which is not a feature in MatNWB but does use external links when doing so.

lawrence-mbf commented 10 months ago

@bendichter @oruebel updated with suggestions