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.

Sign up for GitHub

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

Jump to bottom

Implement find_full_path within ephys modules #35

Merged
merged 25 commits into from
Jan 11, 2022
Merged

Implement find_full_path within ephys modules #35

merged 25 commits into from
Jan 11, 2022

Conversation

kabilar
Copy link
Collaborator

@kabilar kabilar commented Sep 29, 2021 •
edited
Loading

  • And moved functions to element-data-loader.
  • All workflow-array-ephys pytests passed.
  • Rename element-data-loader to element-interface.

@kabilar kabilar requested a review from ttngu207 September 29, 2021 03:56
@kabilar kabilar changed the title Implement find_full_path within ephys module and move functions to element-data-loader Implement find_full_path within ephys modules Sep 29, 2021
ttngu207
ttngu207 approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@ttngu207 ttngu207 left a comment

Choose a reason for hiding this comment

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

Looks good, if anything I'd just suggest a minor change:

from element_data_loader.utils import find_root_directory, find_full_path, dict_to_uuid

just to cut down the element_data_loader.utils. everywhere.

But I don't have a strong preference on this

@kabilar
Copy link
Collaborator Author

kabilar commented Oct 4, 2021

Thanks @ttngu207. Changes implemented. Ready for review and merge.

@kabilar
Copy link
Collaborator Author

kabilar commented Oct 4, 2021 •
edited
Loading

@ttngu207 In this pull request, I also tested your changes for the case of a missing fileTimeSecs in the SpikeGLX meta file. Fix #37.

@kabilar kabilar mentioned this pull request Dec 29, 2021
Closed
CBroz1
CBroz1 approved these changes Dec 30, 2021
View reviewed changes
element_array_ephys/ephys.py Show resolved Hide resolved
element_array_ephys/ephys_chronic.py Show resolved Hide resolved
element_array_ephys/readers/openephys.py Show resolved Hide resolved
dimitri-yatsenko
dimitri-yatsenko reviewed Dec 30, 2021
View reviewed changes
element_array_ephys/ephys.py
Comment on lines 200 to 202
for serial_number, probe_data in dataset.probes.items():
if str(serial_number) == inserted_probe_serial_number:
break
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
for serial_number, probe_data in dataset.probes.items():
if str(serial_number) == inserted_probe_serial_number:
break
for serial_number in dataset.probes:
if str(serial_number) == inserted_probe_serial_number:
break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The variable probe_data is used in the next section after the for loop, so we would need to keep this statement as is.

Copy link
Member

Choose a reason for hiding this comment

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

Then I would get probe_data after the loop

Suggested change
for serial_number, probe_data in dataset.probes.items():
if str(serial_number) == inserted_probe_serial_number:
break
for serial_number in dataset.probes:
if str(serial_number) == inserted_probe_serial_number:
probe_data = dataset.probes[serial_number]
break

@CBroz1 CBroz1 linked an issue Dec 30, 2021 that may be closed by this pull request
Permit multiple root directories for ephys_chronic and ephys_no_curation #49
Closed
CBroz1
CBroz1 reviewed Dec 30, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
CBroz1 and others added 3 commits December 30, 2021 16:34
@kabilar kabilar mentioned this pull request Jan 11, 2022
Closed
@kabilar
Copy link
Collaborator Author

kabilar commented Jan 11, 2022

Added

  • Rename element-data-loader to element-interface

@dimitri-yatsenko dimitri-yatsenko merged commit 088093d into datajoint:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CBroz1 CBroz1 CBroz1 approved these changes

@ttngu207 ttngu207 ttngu207 approved these changes

@dimitri-yatsenko dimitri-yatsenko dimitri-yatsenko approved these changes

Assignees

@kabilar kabilar

@CBroz1 CBroz1

@ttngu207 ttngu207

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Permit multiple root directories for ephys_chronic and ephys_no_curation
4 participants
@kabilar @dimitri-yatsenko @CBroz1 @ttngu207