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

Create Table and helper functions to get position interval name from Interval list key and/or epoch #620

Merged
merged 7 commits into from
Aug 9, 2023

Conversation

samuelbray32
Copy link
Collaborator

Addresses issue of position interval not always being "pos {epoch-1} valid times".

  • Introduces a new table PositionIntervalMap in common_behav composed of IntervalList primary keys and position_interval_name secondary key
  • Updates the convert_epoch_interval_name_to_position_interval_name function from the decoding core to make use of this table and moves it to common_behav. Function populates PositionIntervalMap if map is not found, meaning shouldn't need to populate table elsewhere if you use this function.
  • convert_epoch_interval_name_to_position_interval_name can find position interval using nwb_file_name and either epoch or `interval_list_name'
  • Cleanup cases where position interval defined assuming epoch-1 with calls to this function

@samuelbray32 samuelbray32 requested a review from edeno August 9, 2023 17:25
@edeno edeno requested a review from CBroz1 August 9, 2023 17:26
@samuelbray32 samuelbray32 requested review from dpeg22 and removed request for CBroz1 August 9, 2023 17:26
Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

Seems good to me. I had some comments which are more just things to think about but I didn't see anything that I'm sure would fail.

pos_valid_times[0][0],
pos_valid_times[-1][-1],
] # [pos valid time interval start, pos valid time interval end]
if (time_interval[0] < pos_time_interval[0]) and (
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the assumption here is that the position time interval is always contained within the interval (with some error). I think we have some other code just checking for any overlap in the intervals (in common interval) which might be more robust?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah looking for values from interval_list_intersect would be an easy way to check if there's any overlap. I don't know if there's use cases where you would want it to return a value for partial overlap, or scenarios where there could be multiple small position intervals in an epoch though? This solution seemed the most strict but more likely to give 1:1 mapping

Copy link
Collaborator

Choose a reason for hiding this comment

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

There can be gaps in the position intervals due to loss of tracking but I agree that it probably won't be a problem



def get_pos_interval_list_names(nwb_file_name):
return [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to note a different way this has been done (via regex which I don't necessarily like):

pos_valid_times = (
interval_list.set_index("interval_list_name")
.filter(regex=r"^pos \d+ valid times$", axis=0)
.valid_times
).sort_index(key=lambda index: [int(name.split()[1]) for name in index])

Copy link
Collaborator

@dpeg22 dpeg22 left a comment

Choose a reason for hiding this comment

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

Any chance you've tested this on Abhilasha's NWB file with only run epochs?

@schema
class PositionIntervalMap(dj.Computed):
definition = """
-> IntervalList
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth having a dependence on TaskEpoch to have epoch as a primary key as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if it makes any use cases trickier. It would require you to know the epoch at population, which may not be as obvious for derived interval_list_names (?). Epoch could also have a one:many relationship with interval_list_name, so you'd still need a way to select which to use when searching by epoch


# Check that each pos interval was matched to only one epoch
if len(matching_pos_interval_list_names) > 1:
print(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could potentially raise an Exception here instead

@samuelbray32 samuelbray32 merged commit e949442 into master Aug 9, 2023
@edeno edeno deleted the pos_interval_map branch October 18, 2023 18:01
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.

Match up pos and epoch interval names based on overlapping valid times
3 participants