Apply restrictions on parent tables in fetch_nwb

[samuelbray32]

Description

• Merge table efficiency #1017 created an issue in Merge.fetch_nwb() where if the passed restriction key applied to the parent table rather than the merge table, it wouldn't apply to the fetch_nwb call.

• Issue stems from this line, where restriction would have no effect if not specifying on the Merge table (e.g. if key is not for merge_id):

source_restr = (
                self & {self._reserved_sk: source} & restriction
            ).fetch("KEY")

• This PR enables restrictions on the merge or parent table in Merge.fetch_nwb(restriction)

Checklist:

• No This PR should be accompanied by a release: (yes/no/unsure)
• NA If release, I have updated the CITATION.cff
• No This PR makes edits to table definitions: (yes/no)
• NA If table edits, I have included an alter snippet for release notes.
• NA If this PR makes changes to position, I ran the relevant tests locally.
• I have updated the CHANGELOG.md with PR number and description.
• NA I have added/edited docs/notebooks to reflect the changes

[CBroz1]

I don't love the use of 'either self-restr or parent-restr will work' from the perspective that the function should be transparent about what it accepts and should probably do one or the other. I can see from the existing docstring that the intent was to accept parent restr, so we should fix the existing bug.

dicts have the property of not restricting when invalid, but invalid strings will return an empty set. Will this work for strings? Or does it depend on the null-restr effect of dicts?

[samuelbray32]

I can see from the existing docstring that the intent was to accept parent restr, so we should fix the existing bug.

Yeah, I think one of the early concessions made for user ease with the merge tables was this ability to restrict through for fetching. I wasn't aware about limitations of dicts vs strings. I'll test through permutations

[samuelbray32]

Thanks for catching the string restriction case @CBroz1. I have what I think is a working solution up now. I was testing for these cases:

from spyglass.position.position_merge import PositionOutput
from spyglass.position.v1 import TrodesPosV1

merge_dict = (PositionOutput() & {"source":"TrodesPosV1"}).fetch("KEY",limit=1)[0]
merge_str = (PositionOutput() & merge_dict).restriction[0]
parent_dict =(PositionOutput().merge_get_parent(merge_dict)).fetch1("KEY")
parent_str = (TrodesPosV1() & parent_dict).restriction[0]


for restriction in [merge_dict,merge_str,parent_dict,parent_str]:
    print(restriction)
    (PositionOutput() & restriction).fetch_nwb()
    PositionOutput().fetch_nwb(restriction)[0]

The only case that doesn't run now is (PositionOutput() & parent_str).fetch_nwb() or (PositionOutput() & parent_dict).fetch_nwb() since the restriction doesn't pass through to the function in that case. I think that would fall under improper usage of the function though unless you have another idea?

[CBroz1]

The only case that doesn't run now is (PositionOutput() & parent_str).fetch_nwb() or (PositionOutput() & parent_dict).fetch_nwb() since the restriction doesn't pass through to the function in that case. I think that would fall under improper usage of the function though unless you have another idea?

Table & restr syntax really implies that restr is a valid restriction of Table. I don't think I would go out of my way to permit Table & parent_restr. Though it should pass through based on the func itself ... I'll do some digging

[samuelbray32]

Though it should pass through based on the func itself

Sorry, that was poorly phrased on my part. What was that (Merge() & parent_restriction).restriction does not return parent_restriction, and so parent_restriction could not be accessed in the function if attempted to call this way.

I agree that this isn't a case we should try to make work