Formulate List[StateTransitionGraph] in terms of a class

[redeboer]

I think it would be safer to wrap the allowed transitions (currently represent by a list of StateTransitionGraphs) into a class, something like StateTransition. This object can then contain more advanced methods to give info about the list of StateTransitionGraphs it contains, as well as some checks when adding new StateTransitionGraphs (for instance to enforce that the initial state and final state particles are consistent).

The idea is that a StateTransition instance becomes the product that the reaction module delivers.

[redeboer]

@spflueger moving the discussion of https://github.com/ComPWA/expertsystem/pull/328#pullrequestreview-512083599 here for now

The Result class currently provides:

1. solutions (List[StateTransitionGraph[ParticleWithSpin]])
2. violated_rules
3. not_executed_rules
4. formalism_type (fishy that this would be necessary when computing allowed transitions, but this is another discussion)

If I understand correctly, only 1. and 4. are useful once there are solutions, while 2. and 3. are of use only when there are no solutions. To me that suggests it's better that what is currently the Result class becomes a class as suggested in this issue (a wrapper around List[StateTransitionGraph]) and that we raise a dedicated exception class (NoSolutionsFound or something) that carries violated_rules and not_executed_rules.

[redeboer]

The formalism_type can still be contained within the returned class in case there are solutions, however, I still prefer if we get rid of formalism_type in the reaction module eventually.

[spflueger]

The formalism_type can still be contained within the returned class in case there are solutions, however, I still prefer if we get rid of formalism_type in the reaction module eventually.

I thought about this a bit more. I think the only thing that makes this difficult is that the spin projections have different meanings in the different formalisms. So in the helicity formalism its helicities, while in the canonical its spin projections onto some fixed axis.

To generate a result that is "independent" of the formalism, which could be put into any of the amplitude builders. I currently do not see a way around this other than computing the spin projections in parallel. Or some kind of adapter would be needed to convert.

The good thing is that we currently do not support the canonical formalism natively, but put the canonical formalism on top of the helicity formalism (substituting helicity amps with a sum of canonical ones). Hence we only have the helicity spin projections and do not run into the problem above. Long term this problem will arise though.

[spflueger]

@spflueger moving the discussion of #328 (review) here for now

The Result class currently provides:

1. solutions (List[StateTransitionGraph[ParticleWithSpin]])
2. violated_rules
3. not_executed_rules
4. formalism_type (fishy that this would be necessary when computing allowed transitions, but this is another discussion)

If I understand correctly, only 1. and 4. are useful once there are solutions, while 2. and 3. are of use only when there are no solutions. To me that suggests it's better that what is currently the Result class becomes a class as suggested in this issue (a wrapper around List[StateTransitionGraph]) and that we raise a dedicated exception class (NoSolutionsFound or something) that carries violated_rules and not_executed_rules.

Generally good concept. I don't like the code raising exceptions on common or natural cases. Not executed rules should never happen (i dont really have proof for this, but thats at least to expect as a user). Hence an exception makes perfect sense here. However violated rules are very common, and also part of the responsibility of the framework. I think it would be much nicer to have this information in the "result" somehow instead of raising an Error for that

[redeboer]

Additional benefit: StateTransitionGraph[ParticleWithSpin] can't be 'pickled' because it uses Generic. ComPWA/expertsystem#519 mitigates it, but notes this problem in the documentation.

Still, it would be better to keep StateTransitionGraph[ParticleWithSpin] internal and have the reaction module return some frozen instance. If that output class is written with attrs, that would simplify ComPWA/expertsystem#519 further, because the serialization can be done with attr.asdict (just like it's being done for asdict on ParticleCollection).