nanograv / enterprise

ENTERPRISE (Enhanced Numerical Toolbox Enabling a Robust PulsaR Inference SuitE) is a pulsar timing analysis code, aimed at noise analysis, gravitational-wave searches, and timing model analysis.
https://enterprise.readthedocs.io
MIT License
64 stars 65 forks source link

Class factories have the same name as the classes they return #344

Closed vhaasteren closed 4 months ago

vhaasteren commented 1 year ago

Right now, class factories often have the same name as the class they return. In my PR for fast Kernel Ecorr (#343) I need a mechanism to check whether a class factory has been used before to show a warning message to the user only once. A decorator is a great way to do so. But the name collision used in Enterprise prevents that. Instead of jumping through hoops and do things the wrong way, I think it's better to use a better naming convention in Enterprise. Example that illustrates what goes wrong:

try:
    import fastshermanmorrison.fastshermanmorrison as fastshermanmorrison
except ImportError:
    fastshermanmorrison = None

import logging
logger = logging.getLogger(__name__)

class with_firstrun_counter:
    def __init__(self, func):
        self.func = func
        self.n_called = 0

    def __call__(self, *args, **kwargs):
        self.n_called += 1
        return self.func(*args, **kwargs)

    @property
    def first_run(self):
        return self.n_called == 1

@with_firstrun_counter
def EcorrKernelNoise():

    if EcorrKernelNoise.first_run and not fastshermanmorrison:
        logger.warning("Warning: install fastshermanmorrison for faster Kernel Ecorr")

    class EcorrKernelNoise(object):

        def __init__(self):
            pass

    return EcorrKernelNoise

if __name__=="__main__":

    EKN = EcorrKernelNoise()
    myobject = EKN()

    EKN = EcorrKernelNoise()
    myobject = EKN()

This causes an UnboundLocalError. By adding an underscore, like class EcorrKernelNoise_(object), the code runs. Although valid Python in principle, the pattern used in Enterprise causes name collisions, and is best avoided.

vhaasteren commented 4 months ago

Closing. It's a good point, but not necessary to address