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

Restructure I3Extractor class to make it more modular and object-oriented #30

Merged
merged 19 commits into from
Oct 12, 2021

Conversation

asogaard
Copy link
Collaborator

In this PR, I have proposed a restructuring of I3Extractor that does a few things, most notably:

  • It makes it more object-oriented. This means that as many variables as possible are set as attributes of the class instead of being passed as arguments to each call of the class' main method (__call__). This should make it more clear to the user what is unique for each call (i.e. the physics frame) and what is common across frames (e.g. GCD information, pulse map), and additionally might remove a small overhead in each call. This now means that I3Extractor classes are called with just a single argument, frame.
  • It replaces string statements and use of the built-in eval functions with explicit declarations (e.g. energy = MCInIcePrimary.energy instead of energy = eval("MCInIcePrimary.energy")). This should make it easier for the python interpreter to detect any syntax errors, and it should make it easier for the reader to parse as variable references are now explicit.
  • It replaces the use of a mode flag with specialised classes (currently I3FeatureExtractor, I3RetroExtractor, and I3TruthExtractor, all inheriting from the same base class, I3Extractor). This reduces the complexity of the current I3Extractor class by breaking its functionality down into smaller, more coherent chunks. It also makes the use of I3Extractors more modular, such that just I3FeatureExtractor can be used in "production" (i.e. as part of reconstruction chains) while several may be used for generating intermediate files for training GNN models.
  • It drops the custom_truth argument to I3Extractor. This means that new/alternative variable configurations cannot be specified at runtime; instead, they need to be defined as separate classes. This is intentional, to reduce the complexity for ML developer users while still allowing for extensibility in the form of new I3Extractor-inheriting classes. Let me know if you are using this features, @RasmusOrsoe, and would like to see it retained.

Closes #19.

@asogaard asogaard self-assigned this Sep 30, 2021
@asogaard asogaard added this to Review in Initial repository setup via automation Sep 30, 2021
…ber and order of I3Extractors in DataConverter, since the internal class logic assumes it
Copy link
Collaborator

@RasmusOrsoe RasmusOrsoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

I do think removing the ability to easily adjust to the code to extract an additional item from the frames is a bad idea. I cannot count how many times I've needed the code to do just that.

I can approve this now because we want to move ahead but I do think we want to revisit this particular choice soon.

@RasmusOrsoe RasmusOrsoe merged commit 5d3d66c into graphnet-team:main Oct 12, 2021
Initial repository setup automation moved this from Review to Done Oct 12, 2021
@asogaard asogaard deleted the i3extractor branch October 19, 2021 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Make I3Extractor more object-oriented
2 participants