Decoding qol updates

[samuelbray32]

Description

Resolves #1197

• Allows DecodingParameters fetch and fetch1 to accept heading arguments

Implements full_key_decorator

• Utility function that can be used to decorate class methods of datajoint table classes
• Designed to allow user to call functions such as fetch_spike_data(key) without key containing all primary key elements as long as key specifies a unique database entry

Change previously staticmethods to classmethods in clusterless pipleline

• tables
  • SortedSpikesDecodingV1
  • ClusterlessDecodingV1
  • SortedSpikesGroup
• lets us add the `full_key_decorator to these functions
• implemented to ensure that still works when called with full key and no existing entry (e.g. during calls in the make function

Checklist:

• N This PR should be accompanied by a release: (yes/no/unsure)
• NA If release, I have updated the CITATION.cff
• N 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

[edeno]

Is this a draft or is it ready for review?

[samuelbray32]

Still a draft. I wanted to put in some other helper fixes

[CBroz1]

Hi @samuelbray32 - It's not clear to me what motivated using a decorator over adding a helper method to the mixin. A decorator might grant you this functionality to a non-table method, or classes that don't inherit the mixin, but it seems like that's the case here. It's also not clear to me why the methods you use it on require classmethod. Are there pieces of the funcs above that access the uninstanced class?

As a general rule, I'd like to be able to accept string restrictions anywhere we might accept a key as a dict, but I understand its more hoops

class SpyglassMixin:
    ...

    def get_single_entry_key(
        self, key: dict = None, required_fields: list[str] = None
    ) -> dict:
        if key is None:
            return dict()  # I like explicit dict() over {}, bc {} could be set

        required_fields = required_fields or self.primary_key
        if isinstance(key, (str, dict)):  # check is either keys or substrings
            if not all(field in key for field in required_fields):
                raise ValueError(
                    f"Key must contain all required fields: {required_fields}"
                )

        if len(query := self & key) == 1:
            return query.fetch("KEY", as_dict=True)[0]

        raise ValueError(
            f"Key must identify exactly one entry in {self.camel_name}: {key}"
        )


class SortedSpikesDecodingV1(SpyglassMixin, dj.Computed):
    ...

    def fetch_environments(self, key):
        key = self.get_single_entry_key(key, ["decoding_param_name"])

[samuelbray32]

Thanks Chris:

• I wrote it as a decorator because my original plan was that it would require no additional arguments and simply fetch the fully key every time before running the function. However, several of the cases where it would be useful need to work prior to key insertion in the calling table, requiring a check if the existing key is sufficient before calling fetch. I agree that the decorator isn't as clean anymore and can move it into a mixin method.

• With respect to the use of classmethod, I did that for functions that were previously staticmethod. I was trying to make least amount of change to the existing while still letting me add the utility. For those methods, I'm assuming they were made static because they are used in the make function before the key is inserted into the table.

• For string restrictions, several of these functions already assume that the passed key is a dict. This utility method could handle case of strings by fetching the key from the table, which would generalize the accepted restrictions of the existing functions

[CBroz1]

Makes sense. To keep down the complexity of the codebase overall, I think I'd avoid decorators until we find a case where there's no other way around it

I do wish we had a better pattern for when we do and don't use classmethods. As is, it seems pretty unpredictable for the end user whether to use Table.method(arg) or Table().method(arg). I've tried to default to the latter whenever possible, without classmethod, and reserve the latter for only when the method is doing something at the DJ class level. No reason to change an existing method. Is 'before insertion' the convention you've used?

[samuelbray32]

That's the convention I inferred from the code. Particularly in cases where the function is interacting with upstream tables or in the make function, since those are things that could be called in any future new version of the table

[CBroz1]

Small edit to the error message, but functionally looks good