kymata-atlas / kymata-core

Core Kymata codebase, including statistical analysis and plotting tools
https://kymata.org
MIT License
5 stars 0 forks source link

Unpredictable "magic" behaviour in io.function.load_function #234

Open caiw opened 6 months ago

caiw commented 6 months ago

E.g.

if func_name in ('STL', 'IL', 'LTL'):
    func = func.T

This looks like it was added to address a bug caused by inconsistent structures between different versions of function names. However this has the potential to create extremely difficult-to-find bugs in future.

Same goes for:

if 'neurogram' in func_name:
    # ...
    if func_name == 'neurogram_mr':
        # ...

(although this is so obscure it's less likely to use bugs.)

If we need lots of special-case code like this, it's better to separate it out into specialised functions, or better yet, separate classes with default behaviour and overridden behaviour.

E.g. could have a generic load_function() function, and then have a

def load_legacy_stl(...):
    func = load_function(...)
    if func_name in ('STL', 'IL', 'LTL'):
        func = func.T
    return func

(or whatever). That way, someone using the base load_function will never unexpectedly run into issues which make no sense and where errors don't get raised near the code which caused them.

A related issue (which I'll fold into this one for now but can be split off if necessary), is the presence of arguments like bruce_neurons in the signature, which are relevant to only a subset of cases. Again better to have a specialise function like:

def load_function_neurogram(..., bruce_neurons):
   func = load_func(...)
   #... do extra stuff to do with neurgram interpolation
   return func
neukym commented 4 months ago
if func_name in ('STL', 'IL', 'LTL'):
    func = func.T

has been removed in main, so there are only two issues remaining here.