Misc Issues (#903)

* #892

* #885

* #879

* Partial address of #860

* Update Changelog

* Partial solve of #886 - Ask import

* Fix failing tests

* Add note on order of inheritace

* #933

* Could not replicate fill_nan error. Reverting except clause

.github/ISSUE_TEMPLATE/feature_request.md

@@ -8,13 +8,15 @@ assignees: ''
 ---
 
 **Is your feature request related to a problem? Please describe.**
-A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
+A clear and concise description of what the problem is.
+For example, I'm always frustrated when [...]
 
 **Describe the solution you'd like**
 A clear and concise description of what you want to happen.
 
 **Describe alternatives you've considered**
-A clear and concise description of any alternative solutions or features you've considered.
+A clear and concise description of any alternative solutions or features you've
+considered.
 
 **Additional context**
 Add any other context or screenshots about the feature request here.

.github/pull_request_template.md

@@ -5,15 +5,33 @@ include relevant motivation and context. Please list issues fixed or closed by
 This PR.
 
 - Fixes #000: How this PR fixes the issue
-  - `path/file.py`: Description of the change
-  - `path/file.py`: Description of the change
+    - `path/file.py`: Description of the change
+    - `path/file.py`: Description of the change
 - Fixes #000: How this PR fixes the issue
-  - `path/file.py`: Description of the change
-  - `path/file.py`: Description of the change
+    - `path/file.py`: Description of the change
+    - `path/file.py`: Description of the change
 
 # Checklist:
 
+<!--
+For items below with choices, select one (e.g., yes, no) 
+and check the box to indicate that a choice was selected.
+If not applicable, check the box and add `N/A` after the box.
+For example:
+- [X] This has a choice: no
+- [X] N/A. If choice, other item.
+This comment may be deleted on submission.
+
+Alter notes example:
+```python
+from spyglass.example import Table
+Table.alter() # Comment regarding the change
+```
+-->
+
 - [ ] This PR should be accompanied by a release: (yes/no/unsure)
-- [ ] (If release) I have updated the `CITATION.cff`
-- [ ] I have updated the `CHANGELOG.md`
+- [ ] If release, I have updated the `CITATION.cff`
+- [ ] This PR makes edits to table definitions: (yes/no)
+- [ ] If table edits, I have included an `alter` snippet for release notes.
+- [ ] I have updated the `CHANGELOG.md` with PR number and description.
 - [ ] I have added/edited docs/notebooks to reflect the changes

CHANGELOG.md

@@ -2,12 +2,20 @@
 
 ## [0.5.2] (Unreleased)
 
+### Release Notes
+
+<!-- Running draft to be removed immediately prior to release. -->
+
 ### Infrastructure
 
 - Refactor `TableChain` to include `_searched` attribute. #867
 - Fix errors in config import #882
 - Save current spyglass version in analysis nwb files to aid diagnosis #897
 - Add pynapple support #898
+- Update PR template checklist to include db changes. #903
+- Avoid permission check on personnel tables. #903
+- Add documentation for `SpyglassMixin`. #903
+- Add helper to identify merge table by definition. #903
 - Prioritize datajoint filepath entry for defining abs_path of analysis nwbfile #918
 - Fix potential duplicate entries in Merge part tables #922
 
@@ -37,7 +45,7 @@
     - Fixes to `_convert_mp4` #834
     - Replace deprecated calls to `yaml.safe_load()` #834
 - Spikesorting:
-    - Increase`spikeinterface` version to >=0.99.1, <0.100 #852
+    - Increase`spikeinterface` version to >=0.99.1, \<0.100 #852
     - Bug fix in single artifact interval edge case #859
     - Bug fix in FigURL #871
 - LFP
@@ -214,3 +222,5 @@
 [0.4.2]: https://github.com/LorenFrankLab/spyglass/releases/tag/0.4.2
 [0.4.3]: https://github.com/LorenFrankLab/spyglass/releases/tag/0.4.3
 [0.5.0]: https://github.com/LorenFrankLab/spyglass/releases/tag/0.5.0
+[0.5.1]: https://github.com/LorenFrankLab/spyglass/releases/tag/0.5.1
+[0.5.2]: https://github.com/LorenFrankLab/spyglass/releases/tag/0.5.2

docs/src/contribute.md

@@ -45,7 +45,7 @@ By convention, an individual pipeline has one or more the following table types:
 - Parameters table
 - Selection table
 - Data table
-- Merge Table (see also [doc](./misc/merge_tables.md)
+- Merge Table (see also [doc](./misc/merge_tables.md))
 
 ### Common/Multi-pipeline
 
@@ -226,16 +226,8 @@ faulty connection.
 
 - During development, we suggest using a Docker container. See
     [example](./notebooks/00_Setup.ipynb).
-- DataJoint is unable to set delete permissions on a per-table basis. If a user
-    is able to delete entries in a given table, she can delete entries in any
-    table in the schema. The `SpikeSorting` table extends the built-in `delete`
-    method to check if the username matches a list of allowed users when
-    `delete` is called. Issues #226 and #586 track the progress of generalizing
-    this feature.
 - `numpy` style docstrings will be interpreted by API docs. To check for
     compliance, monitor the std out when building docs (see `docs/README.md`)
-- `fetch_nwb` is currently reperated across many tables. For progress on a fix,
-    follow issue #530
 
 ## Making a release
 

docs/src/misc/merge_tables.md

@@ -10,23 +10,6 @@ and related discussions
 [here](https://github.com/datajoint/datajoint-python/issues/151) and
 [here](https://github.com/LorenFrankLab/spyglass/issues/469).
 
-**Note:** Deleting entries upstream of Merge Tables will throw errors related to
-deleting a part entry before the master. To circumvent this, you can add
-`force_parts=True` to the
-[`delete` function](https://datajoint.com/docs/core/datajoint-python/0.14/api/datajoint/__init__/#datajoint.table.Table.delete)
-call, but this will leave and orphaned primary key in the master. Instead, use
-`(YourTable & restriction).delete_downstream_merge()` to delete master/part
-pairs. If errors persist, identify and import the offending part table and rerun
-`delete_downstream_merge` with `reload_cache=True`. This process will be faster
-for subsequent calls if you reassign the your table after importing.
-
-```python
-from spyglass.common import Nwbfile
-
-nwbfile = Nwbfile()
-(nwbfile & "nwb_file_name LIKE 'Name%'").delete_downstream_merge()
-```
-
 ## What
 
 A Merge Table is fundamentally a master table with one part for each divergent

docs/src/misc/mixin.md

@@ -0,0 +1,123 @@
+# Spyglass Mixin
+
+The Spyglass Mixin provides a way to centralize all Spyglass-specific
+functionalities that have been added to DataJoint tables. This includes...
+
+- Fetching NWB files
+- Delete functionality, including permission checks and part/master pairs
+- Export logging. See [export doc](export.md) for more information.
+
+To add this functionality to your own tables, simply inherit from the mixin:
+
+```python
+import datajoint as dj
+from spyglass.utils import SpyglassMixin
+
+schema = dj.schema('my_schema')
+
+@schema
+class MyOldTable(dj.Manual):
+    pass
+
+@schema
+class MyNewTable(SpyglassMixin, dj.Manual):)
+    pass
+```
+
+**NOTE**: The mixin must be the first class inherited from in order to override
+default DataJoint functions.
+
+## Fetching NWB Files
+
+Many tables in Spyglass inheris from central tables that store records of NWB
+files. Rather than adding a helper function to each table, the mixin provides a
+single function to access these files from any table.
+
+```python
+from spyglass.example import AnyTable
+
+(AnyTable & my_restriction).fetch_nwb()
+```
+
+This function will look at the table definition to determine if the raw file
+should be fetched from `Nwbfile` or an analysis file should be fetched from
+`AnalysisNwbfile`. If neither is foreign-key-referenced, the function will refer
+to a `_nwb_table` attribute.
+
+## Delete Functionality
+
+The mixin overrides the default `delete` function to provide two additional
+features.
+
+### Permission Checks
+
+By default, DataJoint is unable to set delete permissions on a per-table basis.
+If a user is able to delete entries in a given table, she can delete entries in
+any table in the schema.
+
+The mixin relies on the `Session.Experimenter` and `LabTeams` tables to ...
+
+1. Check the session and experimenter associated with the attempted deletion.
+2. Check the lab teams associated with the session experimenter and the user.
+
+If the user shares a lab team with the session experimenter, the deletion is
+permitted.
+
+This is not secure system and is not a replacement for database backups (see
+[database management](./database_management.md)). A user could readily
+curcumvent the default permission checks by adding themselves to the relevant
+team or removing the mixin from the class declaration. However, it provides a
+reasonable level of security for the average user.
+
+### Master/Part Pairs
+
+By default, DataJoint has protections in place to prevent deletion of a part
+entry without deleting the corresponding master. This is useful for enforcing
+the custom of adding/removing all parts of a master at once and avoids orphaned
+masters, or null entry masters without matching data.
+
+For [Merge tables](./merge_tables.md), this is a significant problem. If a user
+wants to delete all entries associated with a given session, she must find all
+Merge entries and delete them in the correct order. The mixin provides a
+function, `delete_downstream_merge`, to handle this, which is run by default
+when calling `delete`.
+
+`delete_downstream_merge`, also aliased as `ddm`, identifies all Merge tables
+downsteam of where it is called. If `dry_run=True`, it will return a list of
+entries that would be deleted, otherwise it will delete them.
+
+Importantly, `delete_downstream_merge` cannot properly interact with tables that
+have not been imported into the current namespace. If you are having trouble
+with part deletion errors, import the offending table and rerun the function
+with `reload_cache=True`.
+
+```python
+from spyglass.common import Nwbfile
+
+restricted_nwbfile = Nwbfile() & "nwb_file_name LIKE 'Name%'"
+restricted_nwbfile.delete_downstream_merge(dry_run=False)
+# DataJointError("Attempt to delete part table MyMerge.Part before ...
+
+from spyglass.example import MyMerge
+
+restricted_nwbfile.delete_downstream_merge(reload_cache=True, dry_run=False)
+```
+
+Because each table keeps a cache of downsteam merge tables, it is important to
+reload the cache if the table has been imported after the cache was created.
+Speed gains can also be achieved by avoiding re-instancing the table each time.
+
+```python
+# Slow
+from spyglass.common import Nwbfile
+
+(Nwbfile() & "nwb_file_name LIKE 'Name%'").ddm(dry_run=False)
+(Nwbfile() & "nwb_file_name LIKE 'Other%'").ddm(dry_run=False)
+
+# Faster
+from spyglass.common import Nwbfile
+
+nwbfile = Nwbfile()
+(nwbfile & "nwb_file_name LIKE 'Name%'").ddm(dry_run=False)
+(nwbfile & "nwb_file_name LIKE 'Other%'").ddm(dry_run=False)
+```

src/spyglass/position/v1/position_trodes_position.py

@@ -288,8 +288,13 @@ def make(self, key):
             f"{current_dir.as_posix()}/{nwb_base_filename}_"
             f"{epoch:02d}_{key['trodes_pos_params_name']}.mp4"
         )
+        red_cols = (
+            ["xloc", "yloc"]
+            if "xloc" in raw_position_df.columns
+            else ["xloc1", "yloc1"]
+        )
         centroids = {
-            "red": np.asarray(raw_position_df[["xloc", "yloc"]]),
+            "red": np.asarray(raw_position_df[red_cols]),
             "green": np.asarray(raw_position_df[["xloc2", "yloc2"]]),
         }
         position_mean = np.asarray(
@@ -330,6 +335,7 @@ def convert_to_pixels(data, frame_size, cm_to_pixels=1.0):
 
     @staticmethod
     def fill_nan(variable, video_time, variable_time):
+        # TODO: Reduce duplication across dlc_utils and common_position
         video_ind = np.digitize(variable_time, video_time[1:])
 
         n_video_time = len(video_time)
@@ -338,6 +344,7 @@ def fill_nan(variable, video_time, variable_time):
             filled_variable = np.full((n_video_time, n_variable_dims), np.nan)
         except IndexError:
             filled_variable = np.full((n_video_time,), np.nan)
+
         filled_variable[video_ind] = variable
 
         return filled_variable
@@ -450,4 +457,7 @@ def make_video(
 
         video.release()
         out.release()
-        cv2.destroyAllWindows()
+        try:
+            cv2.destroyAllWindows()
+        except cv2.error:  # if cv is already closed or does not have func
+            pass

src/spyglass/utils/dj_chains.py

@@ -282,7 +282,10 @@ def find_path(self, directed=True) -> OrderedDict:
             if table.isnumeric():  # get proj() attribute map for alias node
                 if not prev_table:
                     raise ValueError("Alias node found without prev table.")
-                attr_map = self.graph[table][prev_table]["attr_map"]
+                try:
+                    attr_map = self.graph[table][prev_table]["attr_map"]
+                except KeyError:  # Why is this only DLCCentroid??
+                    attr_map = self.graph[prev_table][table]["attr_map"]
                 ret[prev_table]["attr_map"] = attr_map
             else:
                 free_table = dj.FreeTable(self._connection, table)

src/spyglass/utils/dj_merge_tables.py

@@ -18,6 +18,30 @@
 RESERVED_PRIMARY_KEY = "merge_id"
 RESERVED_SECONDARY_KEY = "source"
 RESERVED_SK_LENGTH = 32
+MERGE_DEFINITION = (
+    f"\n    {RESERVED_PRIMARY_KEY}: uuid\n    ---\n"
+    + f"    {RESERVED_SECONDARY_KEY}: varchar({RESERVED_SK_LENGTH})\n    "
+)
+
+
+def is_merge_table(table):
+    """Return True if table definition matches the default Merge table.
+
+    Regex removes comments and blank lines before comparison.
+    """
+    if not isinstance(table, dj.Table):
+        return False
+    if isinstance(table, dj.FreeTable):
+        fields, pk = table.heading.names, table.primary_key
+        return fields == [
+            RESERVED_PRIMARY_KEY,
+            RESERVED_SECONDARY_KEY,
+        ] and pk == [RESERVED_PRIMARY_KEY]
+    return MERGE_DEFINITION == re.sub(
+        r"\n\s*\n",
+        "\n",
+        re.sub(r"#.*\n", "\n", getattr(table, "definition", "")),
+    )
 
 
 class Merge(dj.Manual):
@@ -34,21 +58,16 @@ def __init__(self):
         super().__init__()
         self._reserved_pk = RESERVED_PRIMARY_KEY
         self._reserved_sk = RESERVED_SECONDARY_KEY
-        merge_def = (
-            f"\n    {self._reserved_pk}: uuid\n    ---\n"
-            + f"    {self._reserved_sk}: varchar({RESERVED_SK_LENGTH})\n    "
-        )
         if not self.is_declared:
-            # remove comments after # from each line of definition
-            if self._remove_comments(self.definition) != merge_def:
+            if not is_merge_table(self):  # Check definition
                 logger.warn(
-                    "Merge table with non-default definition\n\t"
-                    + f"Expected: {merge_def.strip()}\n\t"
+                    "Merge table with non-default definition\n"
+                    + f"Expected: {MERGE_DEFINITION.strip()}\n"
                     + f"Actual  : {self.definition.strip()}"
                 )
             for part in self.parts(as_objects=True):
                 if part.primary_key != self.primary_key:
-                    logger.warn(
+                    logger.warn(  # PK is only 'merge_id' in parts, no others
                         f"Unexpected primary key in {part.table_name}"
                         + f"\n\tExpected: {self.primary_key}"
                         + f"\n\tActual  : {part.primary_key}"