Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing a new importing scheme #789

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Conversation

fschlueter
Copy link
Collaborator

@fschlueter fschlueter commented Jan 16, 2025

This is a guilty pleasure thing. I dislike the current scheme of importing modules and framework classes and they require to much code.

(For developers)
This PR adds imports into the __init__.py files of the packages NuRadioReco.modules and NuRadioReco.framework. Here are two snippets:

from .base_shower import BaseShower
from .base_station import BaseStation
from .base_trace import BaseTrace
...

and

from .analogToDigitalConverter import analogToDigitalConverter as AnalogToDigitalConverter
from .beamFormingDirectionFitter import beamFormingDirectionFitter as BeamFormingDirectionFitter

Notice that in the latter the name of the class objects is changed to have a capital letter as first letter. This does two things: It allows backwards compatibility (old import statements are still working) and it implements a typical syntax where classes use capital letters (similar as we do in the framework). However, it might introduce some confusion that the same classes are now accessible with two names (one with and one without the capital letter).

(For users)
This new import scheme allows the following code (pseudocode):

from NuRadioReco import framework as fwk, modules

event = fwk.Event(0, 0)
station = fwk.Station(0, 0)

channelNoiseAdder = modules.ChannelNoiseAdder()

....

One major disadvantage is, that now all modules are always loaded which: I) might take more time (although maybe only the first time, afterwards with caching it should be relatively fast) II) makes it a bit more cumbersome to implement modules which use optional dependencies.

Currently I only modified simulation.py to use this new scheme. I think we could implement the new scheme step by step and do not have to do it all at once (since it is backwards compatible)

@fschlueter
Copy link
Collaborator Author

@cg-laser any thoughts? Do you like this idea or not so much? It does hide a little bit what things are - for me that is okay and the benefits in code writing out way this and over disadvantages (maybe there are more I am not aware of yet). @sjoerd-bouma do you have any thoughts as well?

@anelles
Copy link
Collaborator

anelles commented Jan 23, 2025

I really looks to me that we have bigger fish to fry than working on this ...
My problem with this is indeed that it hides and makes it less transparent to new users.

@cg-laser
Copy link
Collaborator

I see the appeal of these changes. It cleans up the code (a bit) and avoids lengthy lines. The main disadvantage is that it requires "magic" init functions that provide somewhat magic functionality. It gets less obvious where a certain module is implemented.
And more important. Now we need the extra step of registering all modules in the init function and as you already said, all modules are always loaded. So far it was no problem to just add more and more modules (in subdirectories). And if a certain user or collaboration needed another dependency, no problem to the other users. This changes now.
The other thing is that user modules are difficult to integrate. Or one still needs to use the "old" syntax to load in a custom module that is not part of the NuRadioReco repository.

@cozzyd
Copy link
Collaborator

cozzyd commented Feb 6, 2025

As I found with the module checker from #813, it is non-trivial to import all modules. At least one is broken on import (due to internal API change), and several require more complex dependencies (e.g. AraRoot or snowshovel). As far as I can tell, snowshovel is not even publicly available so there is no good solution there.

Of course, you can add try around the imports and just ignore modules that don't work or have unmet dependencies, but you have to do that for each one, which is annoying. Another alternative is to use the import replacement from fuckit.py https://github.com/ajalt/fuckitpy?tab=readme-ov-file#as-a-replacement-for-import, though introducing a dependency on fuckit.py may encourage usage elsewhere...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants