add missing run decorators

[cg-laser]

This PR adds missing run decorators to the run function of NuRadioReco modules. Every run() method needs this decorator to be registered. Then, it is saved which modules are applied to data in the output file.

There might be a way to add such a check to our linter, but this is something for another PR.

[sjoerd-bouma]

Thanks Christian! I've added the missing import statements.

Cosmin's suggestion (to make all classes inherit from a 'base' class that raises an error if the run method is not decorated) is also nice (it would also allow to automatically implement skeleton begin/end methods), though it wouldn't necessarily guarantee this wouldn't happen again as not all classes are currently tested in the CI. So I would leave this as a nice-to-have to consider for a future PR.

[cozzyd]

grep-result.txt

I think a few are missing still? Would you be in favor of a runtime exception if @register_run is not called via use of init_subclasses on the Module class?

[cozzyd]

oh doh, nevermind, there is no base module class, this is just a convention?

[cozzyd]

It shouldn't be too hard to write a script that walks through the module directory and verify that every class that has a run method within that module has been registered. I think it requires just adding an attribute from the decorator, and then some magic with os.walk, __import__, inspect, and getattr?

It would also have the advantage of being able to determine if a module is broken :).

[cozzyd]

It looks like I don't have commit access to this repository (lol). Here's a script that walks through all the modules in the modules directory, tries to import them (this might fail if the module is broken or if it depends on optional dependencies you don't have), finds all the classes defined in the module, and looks for undecorated run methods (taking advantage of the fact that the run decorator adds a time attribute). It has to be run from the base directory now, though that easily be fixed...

#! /usr/bin/env python3



# module and try to import (possibly failing if for example, you don't have all
# the optional things for each module)

# Then it will find all classes defined in
# the module with a run method, and complain if the time attribute is not
# present (this is an attribute added by the register_run decorator)

import os
import inspect
import importlib

broken = []
unregistered_runs = []

for dirpath,folders,files in os.walk('NuRadioReco/modules'):
    for f in files:
        if not f.endswith(".py") or f == "__init__.py":
            continue
        try:
            mname = dirpath.replace('/','.')+'.'+f[:-3]
            print("Trying ", mname)

            m = importlib.import_module(mname)

            for name,obj in inspect.getmembers(m, lambda member: inspect.isclass(member) and member.__module__ == mname):
                print("Found class ",name, obj)
                if hasattr(obj,'run') and not hasattr(obj.run,'time'):
                    print('Has run method but not registered properly! Public flogging will be scheduled.')
                    unregistered_runs.append(mname + '.' +name)

        except Exception as e:
            print(e)
            print("Couldn't load module... maybe it's broken, oh well")
            broken.append(mname)

print("\n\n\n.........................................\n")
print ("Broken modules: ", broken)
print()
print ("Unregistered runs: ", unregistered_runs)

[cozzyd]

I pushed the missing decorators I found (I checked the ones that didn't work on my system manually). If the CI is such that every module "should work," then I think this would be a useful CI check, but it might be hard to get e.g. ARA, RNO-G and ARIANNA stuff to simultaneously work. A grep-based solution is not susceptible to this, though I think checking that each module can import is a useful thing to do if we can manage. I believe the NuRadioReco.modules.envelope_phasedarray.triggerSimulator module, for example, is broken for reasons not related to importing dependencies.

I also added a check for modules that have a run method but no begin or end. I don't know if this is actually a problem or not (maybe those are meant to be optional?), but I suspect it's better for everything to have a noop begin/end rather than a missing one for consistency. It's definitely not as bad as missing the decorator, obviously, as it doesn't impact anything other than a consistent API.

On my system (I haven't checked non-importing modules yet), I get:

Missing end:  ['NuRadioReco.modules.eventTypeIdentifier.eventTypeIdentifier', 'NuRadioReco.modules.triggerTimeAdjuster.triggerTimeAdjuster', 'NuRadioReco.modules.channelTimeOffsetCalculator.channelTimeOffsetCalculator', 'NuRadioReco.modules.channelAddCableDelay.channelAddCableDelay', 'NuRadioReco.modules.channelGalacticNoiseAdder.channelGalacticNoiseAdder', 'NuRadioReco.modules.channelCWNotchFilter.channelCWNotchFilter', 'NuRadioReco.modules.cosmicRayEnergyReconstructor.cosmicRayEnergyReconstructor', 'NuRadioReco.modules.RNO_G.crRNOGTemplateCreator.crRNOGTemplateCreator', 'NuRadioReco.modules.neutrinoVertexReconstructor.neutrino2DVertexReconstructor.neutrino2DVertexReconstructor', 'NuRadioReco.modules.neutrinoVertexReconstructor.neutrino3DVertexReconstructor.neutrino3DVertexReconstructor', 'NuRadioReco.modules.LOFAR.hardwareResponseIncorporator.hardwareResponseIncorporator']

Missing begin:  ['NuRadioReco.modules.analogToDigitalConverter.analogToDigitalConverter', 'NuRadioReco.modules.efieldToVoltageConverterPerEfield.efieldToVoltageConverterPerEfield', 'NuRadioReco.modules.channelAntennaDedispersion.channelAntennaDedispersion', 'NuRadioReco.modules.ARA.triggerSimulator.triggerSimulator']

[cozzyd]

I guess the script requires setting PYTHONPATH appropriately to run now that it's not in the base directory, but I guess that's not a real problem. FYI, you could have used git mv rather than rm/add to preserve history, but too annoying to change that now :).

I think this looks good, it might in the future be nice to test if modules can import, but it might be too complicated in the CI to ensure all dependencies (particularly ARA and ARIANNA, though I can vouch for the ARA modules passing the checks once I set up AraRoot).

For posterity, here is what I get with almost all dependencies installed (I don't have snowShovel... and all the links I can find are dead... perhaps it exists somewhere?)

Broken modules:
        NuRadioReco.modules.envelope_phasedarray.triggerSimulator
        NuRadioReco.modules.io.snowshovel.readARIANNADataCalib

Unregistered runs:


Missing end:
        NuRadioReco.modules.eventTypeIdentifier.eventTypeIdentifier
        NuRadioReco.modules.triggerTimeAdjuster.triggerTimeAdjuster
        NuRadioReco.modules.channelTimeOffsetCalculator.channelTimeOffsetCalculator
        NuRadioReco.modules.channelAddCableDelay.channelAddCableDelay
        NuRadioReco.modules.channelGalacticNoiseAdder.channelGalacticNoiseAdder
        NuRadioReco.modules.channelCWNotchFilter.channelCWNotchFilter
        NuRadioReco.modules.cosmicRayEnergyReconstructor.cosmicRayEnergyReconstructor
        NuRadioReco.modules.RNO_G.crRNOGTemplateCreator.crRNOGTemplateCreator
        NuRadioReco.modules.iftElectricFieldReconstructor.iftElectricFieldReconstructor.IftElectricFieldReconstructor
        NuRadioReco.modules.neutrinoVertexReconstructor.neutrino2DVertexReconstructor.neutrino2DVertexReconstructor
        NuRadioReco.modules.neutrinoVertexReconstructor.neutrino3DVertexReconstructor.neutrino3DVertexReconstructor
        NuRadioReco.modules.LOFAR.hardwareResponseIncorporator.hardwareResponseIncorporator

Missing begin:
        NuRadioReco.modules.analogToDigitalConverter.analogToDigitalConverter
        NuRadioReco.modules.efieldToVoltageConverterPerEfield.efieldToVoltageConverterPerEfield
        NuRadioReco.modules.channelAntennaDedispersion.channelAntennaDedispersion
        NuRadioReco.modules.ARA.triggerSimulator.triggerSimulator

[sjoerd-bouma]

I guess the script requires setting PYTHONPATH appropriately to run now that it's not in the base directory, but I guess that's not a real problem. FYI, you could have used git mv rather than rm/add to preserve history, but too annoying to change that now :).

I think this looks good, it might in the future be nice to test if modules can import, but it might be too complicated in the CI to ensure all dependencies (particularly ARA and ARIANNA, though I can vouch for the ARA modules passing the checks once I set up AraRoot).

Ah, I didn't realize git mv did something different than just manually moving the file... didn't mean to steal your credit! Thanks for the script.

And yes, it might be nice to make the test fail on broken imports as well, but we'd (at least for now) have to add a list of exceptions. Maybe something for a future PR.