Discussion about improving SPARKX architecture

[NGoetz]

Today, during a software architecture workshop, I started thinking again about our class structure in SPARKX. I think we are at a suboptimal point at the moment.

• Both OSCAR and JETSCAPE have substantial intersections in implementation. We really break the DRY principle badly
• As a result of it, we had multiple times different behavior for both. This is reducing maintainability
• Although all other classes are supposed to work with either, due to OSCAR and JETSCAPE being completely unrelated, we have to work on particle object lists instead of on the object directly
• OSCAR exists in multiple flavors. This is a significant level of complexity, which might increase with the introduction of binary (?)

My proposed action plan for the upcoming sprint for tackling that: We introduce an abstract base class (ABC) ParticleList (name up for discussion) which implements all shared functionality of OSCAR and JETSCAPE. Both OSCAR and JETSCAPE inherite from the ABC (just like we did it for flows already). For all shared methods, there will be one point of truth thanks to this. We change Flow, EventCharacteristics etc. to act on ParticleList. Users will not have to touch the particle object list anymore by hand. This is good because Oscar/Jetscape have the task to load data and, with help of Filter, to filter it. Extracting the particle object list makes people forget that subsequent filters are still acted upon it due to Python magic.

Points to consider and discuss: How should the future of the OSCAR class look? Should OSCARExtended and OSCARIC inherit from OSCAR, or do we want to keep the flag property inside the class? I see pros and cons of both. Should each of these flags have a mirrored binary flag, or should binary be a new flag? Inheritance has the advantage of disentanglement, but also causes some (although limited) duplication.

Please share your opinions on both folks!

[NGoetz]

Additionally, Flow should be a Protocol from typing, because an abstract base class has implemented methods and protocols are interfaces which contain no readily implemented methods, but only placeholders. The placeholders also should raise a NotImplementedError

[NGoetz]

It might be worthwhile to think about which parts of the code are the core, non-volatile parts and which once are expected to change more.

[Hendrik1704]

Just a quick comment here: Jetscape has now different modes for hadrons and partons, so there are also two only slightly different setups there.

The changes you suggest here are probably something, where we can release a SPARKX 2.0.0 if we decide to do this. This does not seem backwards compatible if we implement these changes. We should probably decide this now before the code grows and we have to change too much code in the future...

[NGoetz]

Actually no, all exact changing if the analysis classes act on a particle objects list or a general data type is completely backwards compatible. I fully agree though that this changes are better done earlier than later, as a good and clear architecture is helping with future development.

Regarding JETSCAPE: one could add another layer of abstraction there. Inheritance is nice.

[NGoetz]

One issue: If we change the composition strategy from feeding particle data lists to the other classes to actual Oscar/Jetscape classes, then the tests will also change. This can be potentially dealt with by introducing a mock class concept, which mocks Oscar/Jetscape for the tests, as these are usually initialised from files, and we want to avoid that for testing.

[NGoetz]

Reminder for both me and future reviewers that this will lead to major changes in the documentation too

[NGoetz]

I have implemented a Loader/Storer composition structure on the respective branch for both Oscar/Jetscape. Leftover ToDos:

• [x] Include a third Dummy class for custom online created ParticleLists
• [x] Statically Typing
• [x] Update Documentation and Tests
• [x] Critical Review if the separation of classes is optimal
• [x] Decision regarding ObjectList or BaseStorer interfaces
• [x] Decide if Oscar/Jetscape should be moved into the Storer folder
• [x] Format Code

Due to the recent formatter merge, this will create the merge conflict of the year. We will merge together and pray, once the day has come.

[NGoetz]

This has been merged and finished.