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

Database redesign to add upstream recording schema #24

Merged
merged 34 commits into from
Feb 17, 2022
Merged

Conversation

Alvalunasan
Copy link
Contributor

First draft of recording schema (schema for automatic process of acquisitions. Also serve as "Parent" for ephys/imaging)

Screen Shot 2022-01-20 at 4 01 53 PM

u19_pipeline/recording.py Outdated Show resolved Hide resolved
u19_pipeline/recording.py Outdated Show resolved Hide resolved
Comment on lines 18 to 20
['ephys', '', '/braininit/Data/eletrophysiology'],
['imaging', '', '/braininit/Data/eletrophysiology'],
['video_acquisition', '', '/braininit/Data/video_acquisition']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
['ephys', '', '/braininit/Data/eletrophysiology'],
['imaging', '', '/braininit/Data/eletrophysiology'],
['video_acquisition', '', '/braininit/Data/video_acquisition']
['ephys', 'Neuropixels', '/mnt/cup/braininit/Data/electrophysiology'],
['imaging', 'Two-photon calcium imaging', '/mnt/cup/braininit/Data/imaging'],
['video_acquisition', '', '/mnt/cup/braininit/Data/video_acquisition']

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prepended /mnt/cup. Is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well I was thinking just /braininit is ok to be a reference for the actual path.
/braininit is /mnt/cup/braininit in spock, scotty
/braininit is /Volumes/braininit on Mac
/braininit is \cup.pni.princeton.edu\braininit\ on Windows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. Sounds great.

recording_modality: varchar(64) # modalities for recording (ephys, imaging, video_recording, etc.)
---
modality_description: varchar(255) # description for the modality
root_direcory: varchar(255) # root directory where that modality is stored (e.g. ephys = /braininit/Data/eletrophysiology)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
root_direcory: varchar(255) # root directory where that modality is stored (e.g. ephys = /braininit/Data/eletrophysiology)
root_directory: varchar(255) # root directory where that modality is stored

Since we have the contents property, perhaps we don't need the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed !

u19_pipeline/recording.py Outdated Show resolved Hide resolved
u19_pipeline/recording.py Outdated Show resolved Hide resolved
cls.insert1(param_dict)

@schema
class Recording(dj.Manual):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not effect the functionality of this module, but perhaps the Recording table should be placed directly after the RecordingModality table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right,
My "logic" was to put all lookup tables first.
But your proposal makes a lot of sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Either way works. I am just used to ordering the tables in approximately the same order as how the data flows.

Comment on lines 98 to 100
-> [nullable] acquisition.Session # acquisition Session key
-> [nullable] subject.Subject.proj(acquisition_subject='subject_fullname') # Recording subject when no behavior Session present
recording_datetime=null: datetime # Recording datetime when no bheavior Session present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a difficult task to implement. Perhaps we could use Part tables.

class Recording(dj.Manual):
    definition = """
        recording_id: INT(11) AUTO_INCREMENT
        ---
        -> RecordingModality                         
        recording_directory: varchar(255)
        """
    class BehaviorSession(dj.Part):
        definition = """
        -> master
        ---
        -> acquisition.Session.proj(recording_datetime='session_start_time')
        """
    class RecordingSession(dj.Part):
        definition = """
        -> master
        ---
        -> subject.Subject
        recording_datetime: datetime
        """

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this design is indeed cleaner

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@kabilar kabilar changed the title Db redesign Database redesign to add upstream recording schema Jan 26, 2022
u19_pipeline/recording.py Outdated Show resolved Hide resolved
u19_pipeline/recording.py Outdated Show resolved Hide resolved
u19_pipeline/ephys.py Outdated Show resolved Hide resolved
u19_pipeline/ephys.py Outdated Show resolved Hide resolved
u19_pipeline/ephys.py Outdated Show resolved Hide resolved
Alvalunasan and others added 6 commits January 31, 2022 12:59
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
---
status_definition: VARCHAR(256) # Status definition
"""
contents = get_content_list_from_status_dict()
Copy link
Member

@kabilar kabilar Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we do not need to define a separate function (get_content_list_from_status_dict)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do it without a separate function

u19_pipeline/recording.py Outdated Show resolved Hide resolved
u19_pipeline/ephys.py Outdated Show resolved Hide resolved


# ephys element requires table with name Session
Session = EphysSession
Session = EphysSorting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set Session to EphysRecording or EphysSorting? Not sure what the correct answer is.

u19_pipeline/imaging_rec.py Outdated Show resolved Hide resolved
u19_pipeline/imaging_rec.py Outdated Show resolved Hide resolved
@kabilar
Copy link
Member

kabilar commented Feb 1, 2022

Hi @Alvalunasan, within the TestAutomaticPipeline.ipynb notebook there is a reference to the acquisition.AcquisitionSessionsTestAutoPipeline table but I am not sure where this is declared. Thanks.

Copy link
Member

@kabilar kabilar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @Alvalunasan! Very thorough design. Looking forward to seeing it in action.

Alvalunasan and others added 19 commits February 2, 2022 09:37
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
@Alvalunasan Alvalunasan merged commit 380d497 into master Feb 17, 2022
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