Skip to content
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

Updated Repo for Compatibility w/ DANDI #28

Merged
merged 10 commits into from
Jun 19, 2023

Conversation

pauladkisson
Copy link
Member

Fixes #18

@pauladkisson pauladkisson self-assigned this Jun 15, 2023
@pauladkisson pauladkisson added the documentation Improvements or additions to documentation label Jun 15, 2023
@pauladkisson pauladkisson marked this pull request as draft June 15, 2023 05:35
@pauladkisson pauladkisson changed the title updated metadata.yaml file for compatibility with DANDI Updated Repo for Compatibility w/ DANDI Jun 15, 2023
@pauladkisson pauladkisson marked this pull request as ready for review June 15, 2023 23:40
@pauladkisson
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

pauladkisson commented Jun 17, 2023

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!

@CodyCBakerPhD CodyCBakerPhD merged commit 17a845d into catalystneuro:main Jun 19, 2023
@pauladkisson pauladkisson deleted the update_for_dandiset branch June 20, 2023 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fill Out Dandiset
2 participants