Skip to content

add new raw/scratch script #534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 6, 2023
Merged

add new raw/scratch script #534

merged 11 commits into from
Sep 6, 2023

Conversation

lawrence-mbf
Copy link
Collaborator

@lawrence-mbf lawrence-mbf commented Aug 30, 2023

Motivation

Adds Raw/Scratch tutorial from this pynwb guide

  • Add HDFView image confirming written external link
  • Add link to tutorial in README
  • refactor multiline function format
  • Run files through NWB Inspector

How to test the behavior?

Documentation should be correct and cover the same topics

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

Anon and External Links may point to other group-like objects (like Links and
other wrapper types). CheckDType now resolves this recursively with extra checks
for loops.
@lawrence-mbf lawrence-mbf self-assigned this Aug 30, 2023
@lawrence-mbf lawrence-mbf marked this pull request as ready for review August 31, 2023 14:38
@lawrence-mbf lawrence-mbf marked this pull request as draft August 31, 2023 14:38
@codecov
Copy link

codecov bot commented Aug 31, 2023

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%) ⬆️

... and 2 files with indirect coverage changes

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

@lawrence-mbf lawrence-mbf marked this pull request as ready for review August 31, 2023 14:55
@bendichter
Copy link
Contributor

bendichter commented Aug 31, 2023

@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
Copy link
Collaborator Author

lawrence-mbf commented Aug 31, 2023

@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.

- uses python's black-like multiline argument structure
- Remove extraneous external links section
- Update link text to scratch tutorial
@lawrence-mbf
Copy link
Collaborator Author

@bendichter @oruebel updated with suggestions

@lawrence-mbf lawrence-mbf requested a review from rly September 5, 2023 14:33
@lawrence-mbf lawrence-mbf merged commit 6208428 into master Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants