LorenFrankLab / spyglass

Neuroscience data analysis framework for reproducible research built by Loren Frank Lab at UCSF
https://lorenfranklab.github.io/spyglass/
MIT License
94 stars 43 forks source link

`SpyglassMixinPart` rejects restrictions of self on delete #1163

Open CBroz1 opened 1 month ago

CBroz1 commented 1 month ago

Problem

SpyglassMixinPart.delete was designed to allow propagation of deletes through parts with fk refs, but, in doing so, removes the ability to restrict the part itself by a secondary key.

We could decide that this is working as intended - one should not target the part itself for delete. Or, we can say this is counter intuitive and adjust the delete to accept either restriction of part or master

Example tables

@schema
class MyMaster(SpyglassMixin, dj.Whatever):
    description = """
    master_pk: int
    ---
    master_sk: int
    """

    class MyPart(SpyglassMixinPart):
    description = """
    --> master
    part_pk: int
    ---
    part_sk: int
    """

Example usage

(MyMaster.MyPart & 'master_pk = 1').delete() # works
(MyMaster.MyPart & 'master_sk = 1').delete() # does not work
(MyMaster.MyPart & 'part_pk = 1').delete() # does not work
(MyMaster.MyPart & 'part_sk = 1').delete() # does not work

Should all of these work?

Solutions

  1. Long distance restricting the master in this delete step would do that (i.e., self.master >> restriction) while also opening the door to any deletes downstream and maybe being more overhead than we'd like
  2. A try/except restriction of the part or direct cascade of the restriction to the master would be less costly
edeno commented 1 month ago

Are there downsides to making the last three cases work?

CBroz1 commented 1 month ago

Depending on the implementation, computation time or consistency/violating expectations. Currently, relatively few of the part tables in the package inherit this class. Arguably, they all should