Allow dlc pipeline to run without prior position tracking #970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edeno
approved these changes
May 13, 2024
|
Errors related to #961 showed up with further user use. Converting to a draft until I have those patched |
edeno
approved these changes
May 13, 2024
CBroz1
added a commit
that referenced
this pull request
May 13, 2024
…)" This reverts commit 113ce9a.
CBroz1
reviewed
May 13, 2024
| ### Pipelines | ||
|
|
||
| - DLC | ||
| - Allow dlc without pre-existing tracking data #950 |
There was a problem hiding this comment.
Other lines reflect PR number out of my assumption that a user would want to see the set of changes from the PR, and can follow up on the linked issues for the motivation
Suggested change
| - Allow dlc without pre-existing tracking data #950 | |
| - Allow dlc without pre-existing tracking data #970 |
Comment on lines
+271
to
+281
| if ( | ||
| RawPosition & key | ||
| ): # if spatial series exists, get metadata from there | ||
| spatial_series = (RawPosition() & key).fetch_nwb()[0][ | ||
| "raw_position" | ||
| ] | ||
| reference_frame = spatial_series.reference_frame | ||
| comments = spatial_series.comments | ||
| else: | ||
| reference_frame = "" | ||
| comments = "no comments" |
There was a problem hiding this comment.
Suggested change
| if ( | |
| RawPosition & key | |
| ): # if spatial series exists, get metadata from there | |
| spatial_series = (RawPosition() & key).fetch_nwb()[0][ | |
| "raw_position" | |
| ] | |
| reference_frame = spatial_series.reference_frame | |
| comments = spatial_series.comments | |
| else: | |
| reference_frame = "" | |
| comments = "no comments" | |
| # if spatial series exists, get metadata from there | |
| if query := (RawPosition & key): | |
| spatial_series = query.fetch_nwb()[0]["raw_position"] | |
| else: | |
| spatial_series = None |
Comment on lines
+289
to
+290
| reference_frame=reference_frame, | ||
| comments=comments, |
There was a problem hiding this comment.
Suggested change
| reference_frame=reference_frame, | |
| comments=comments, | |
| reference_frame = getattr(spatial_series, "reference_frame", ""), | |
| comments = getattr(spatial_series, "comments", "no comments"), |
Comment on lines
+156
to
+157
| reference_frame=reference_frame, | ||
| comments=comments, |
There was a problem hiding this comment.
Suggested change
| reference_frame=reference_frame, | |
| comments=comments, | |
| reference_frame = getattr(spatial_series, "reference_frame", ""), | |
| comments = getattr(spatial_series, "comments", "no comments"), |
Comment on lines
+139
to
+149
| if ( | ||
| RawPosition & key | ||
| ): # if spatial series exists, get metadata from there | ||
| spatial_series = (RawPosition() & key).fetch_nwb()[0][ | ||
| "raw_position" | ||
| ] | ||
| reference_frame = spatial_series.reference_frame | ||
| comments = spatial_series.comments | ||
| else: | ||
| reference_frame = "" | ||
| comments = "no comments" |
There was a problem hiding this comment.
Suggested change
| if ( | |
| RawPosition & key | |
| ): # if spatial series exists, get metadata from there | |
| spatial_series = (RawPosition() & key).fetch_nwb()[0][ | |
| "raw_position" | |
| ] | |
| reference_frame = spatial_series.reference_frame | |
| comments = spatial_series.comments | |
| else: | |
| reference_frame = "" | |
| comments = "no comments" | |
| # if spatial series exists, get metadata from there | |
| if query := (RawPosition & key): | |
| spatial_series = query.fetch_nwb()[0]["raw_position"] | |
| else: | |
| spatial_series = None |
CBroz1
reviewed
May 13, 2024
| # check if a position interval exists for this epoch | ||
| try: | ||
| interval_list_name = ( | ||
| convert_epoch_interval_name_to_position_interval_name( |
There was a problem hiding this comment.
It's not clear to me why convert_epoch... should only raise an error in the populate_missing case. I think might add another arg for raise_err over there to avoid try/except. I was taught that try/except only belongs in production if you can't modify the underlying behavior, eg on outside package
There was a problem hiding this comment.
@ CBroz1 agreed. I'll tweak that function
CBroz1
reviewed
May 13, 2024
Comment on lines
+262
to
+264
| pos_time = video_time | ||
| reference_frame = "" | ||
| comments = "no comments" |
There was a problem hiding this comment.
A similar thing could be done here as above with getattr and supplying these as defaults
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #950
Fixes #961
analysis_file_namein make functions before fetching needed dataChecklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.