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

Add Miniscope-DAQ-V4 and CaImAn trigger #13

Merged
merged 73 commits into from
Apr 29, 2022
Merged

Conversation

kabilar
Copy link
Collaborator

@kabilar kabilar commented Oct 18, 2021

  • Combine imaging and scan modules into one miniscope module
  • Move readers to element-interface
  • Add support for Miniscope-DAQ-V4
  • Add support to trigger CaImAn
  • Remove support for MiniscopeAnalysis (since this package is not currently supported).
  • Add GitHub Issue templates
  • Move background.md to elements.datajoint.org
  • Add changelog
  • Add Docker and Compose files
  • Update README
  • Update diagram
  • Also see: Update pipeline declaration, ingestion script, and notebooks workflow-miniscope#18

shenshan and others added 26 commits May 14, 2021 17:39
remove 2p terms and adapt with inscopix miniscope
@kabilar kabilar changed the title [WIP] Change table architecture. Add support for Miniscope-DAQ-V4. Refactor tables to add support for interchangeable processing. Add support for Miniscope-DAQ-V4 and MiniAn. Oct 26, 2021
@kabilar kabilar changed the title Refactor tables to add support for interchangeable processing. Add support for Miniscope-DAQ-V4 and MiniAn. Add support for interchangeable processing, and for Miniscope-DAQ-V4 and MiniAn. Oct 26, 2021
@kabilar kabilar changed the title Add support for interchangeable processing, and for Miniscope-DAQ-V4 and MiniAn. Add support for interchangeable processing, and for Miniscope-DAQ-V4 and MiniAn. Oct 26, 2021
@kabilar kabilar changed the title Add Miniscope-DAQ-V4 and CaImAn trigger Add Miniscope-DAQ-V4 and CaImAn trigger Apr 26, 2022
@kabilar kabilar marked this pull request as ready for review April 26, 2022 16:58
@kabilar
Copy link
Collaborator Author

kabilar commented Apr 26, 2022

Hi @tdincer, this pull request is ready for review. Let's try to merge by Friday, April 29. Thanks!

element_miniscope/miniscope.py Show resolved Hide resolved
element_miniscope/miniscope.py Outdated Show resolved Hide resolved
element_miniscope/miniscope.py Show resolved Hide resolved
element_miniscope/miniscope.py Show resolved Hide resolved
filename_hash = '.' + str(dict_to_uuid(dict(**key, **params)))

if not os.path.isfile(output_dir / filename_hash):
run_caiman(file_paths=avi_files,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing an error here, related to caiman's fit_file:

TypeError: fit_file() got an unexpected keyword argument 'output_dir'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, this feature is currently being evaluated in this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming a release this week, how should we document the current functionality to a potential user who sees the tutorial video next week?


@classmethod
def insert_new_params(cls, processing_method: str, paramset_id: int,
paramset_desc: str, params: dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest optional insert of processing method in parent table. Suggestion below would also require an edit to the parent table insert

I think we also need a docstring here, especially if we're headed in the direction of documentation via site rendering of docstrings.

Suggested change
paramset_desc: str, params: dict):
paramset_desc: str, params: dict,
processing_method_desc: str=''):

Comment on lines 831 to 835
def populate_all(display_progress=True):

populate_settings = {'display_progress': display_progress,
'reserve_jobs': False,
'suppress_errors': False}
Copy link
Contributor

Choose a reason for hiding this comment

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

To put these within the schema is a departure from convention elsewhere. Do we want this in the schema or in a workflow process.py script?

If in the workflow, I would also allow move the reserve_jobs and suppress_errors to the arguments with the same defaults.

Suggested change
def populate_all(display_progress=True):
populate_settings = {'display_progress': display_progress,
'reserve_jobs': False,
'suppress_errors': False}
def populate_all(display_progress=True, reserve_jobs=False, suppress_errors=False):
populate_settings = {'display_progress': display_progress,
'reserve_jobs': reserve_jobs,
'suppress_errors': suppress_errors}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @CBroz1. I would suggest moving this function to the element so that it is not duplicated across each users' workflows. And if you agree, we can then make this update across the other element and workflow repositories.

Good point regarding the arguments. I have committed your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

element_miniscope/miniscope.py Show resolved Hide resolved
element_miniscope/miniscope.py Show resolved Hide resolved
element_miniscope/miniscope.py Outdated Show resolved Hide resolved
element_miniscope/miniscope.py Outdated Show resolved Hide resolved
element_miniscope/miniscope.py Outdated Show resolved Hide resolved
@ttngu207 ttngu207 merged commit de2db40 into datajoint:main Apr 29, 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
6 participants