search

catalystneuro / datta-lab-to-nwb

MIT License
1 stars 2 forks source link

Updated Repo for Compatibility w/ DANDI #28

Closed pauladkisson closed 1 year ago

pauladkisson commented 1 year ago

Fixes #18

pauladkisson commented 1 year ago

This PR should now address all the best practice critical/violations/suggestions from nwbinspector EXCEPT

  1. check_roi_response_series_link_to_plane_segmentation but see nwbinspector Issue
  2. check_single_row for fluorophores and photodetectors, but there's only 1 fluorophore and 1 photodetector used in these experiments so I think it's fine.
pauladkisson commented 1 year ago

I also moved the example nwbfile to inside this github repo. I know it's generally better practice to keep large data files separate, but this one session is only ~3MB, and it seems handy to have a version-controlled example file for inspection.

CodyCBakerPhD commented 1 year ago

I also moved the example nwbfile to inside this github repo. I know it's generally better practice to keep large data files separate, but this one session is only ~3MB, and it seems handy to have a version-controlled example file for inspection.

Please remove that and continue to share it outside the repo

If email is too inconvenient, we can use either a shared Google Drive, Dropbox, or the 'catalystneuro-processing' S3 bucket (which I believe you have credentials to access?), or TBH at this point we are good to upload any draft files to the actual DANDI set

Reason: The current size of the entire datta-lab-to-nwb history is 2.6 MB by my calculation, with the largest file by commit being ~10 KB. The point is not that the literal size of the file is only 3 MB (though that is also excessive to more than double a repository size just for that), but rather the bad practice is the precedence for establishing version control for that item in the first place, which would inflate the repo size by that file size multiplied by the number of individual commits changing that file.

The only files we've ever made an exception for in that regards are NWB files that are on the order of single-double digit KB and so are practically indistinguishable from the text files they live next to

With regards to your ideas for a CI - the action itself would handle data transfer from source to cache, which is fine because those logs are not version controlled and are even cleared automatically every couple of months

Note to self: do not forget to squash this PR so that it gets purged from the history

pauladkisson commented 1 year ago

Ok, I removed the nwbfile, so this should be good to merge now. I will look into some of the alternative solutions we discussed (ex. CI streaming from DANDI) when I get a chance, but probably not until after 6/26/23.

Note to self: do not forget to squash this PR so that it gets purged from the history

Don't forget!