Rename Particle class

[apuignav]

Should we rename Particle to avoid name clashes with https://github.com/scikit-hep/particle?

@eduardo-rodrigues Suggestions?

[eduardo-rodrigues]

Hi folks, I will try and provide some feedback soon.

For now I just installed the package on my Windows laptop. All went fine. I copy-and-pasted the first example on the README but it failed. Turns out phasespace.generate no longer exist in the latest API … May I suggest that you review your README and doc ;-)?

Another little suggestion: personally, I like to see the dependencies listed in READMEs. In https://github.com/scikit-hep/decaylanguage we even separate required from recommended. Maybe you can consider adding a little paragraph like that?

[jonas-eschle]

Hi, thanks for the report, I've fixed it in the docs, it's updated in the master. It's now generate_decay instead of generate.

Yes, we could list them, I usually don't mind. Right around the installation instructions. No strong feeling though . @apuignav ?

[apuignav]

Why not, let's do it.

[eduardo-rodrigues]

So far I did not manage to find a better name than GenParticle.

BTW, I miss 2 little things in the GenParticle class. Let me type some things I would expect:

In [8]: pion._mass
Out[8]: <tf.Tensor 'Const:0' shape=() dtype=float64>

In [9]: pion.mass
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-9-2d6ad1142b1a> in <module>
----> 1 pion.mass

AttributeError: 'Particle' object has no attribute 'mass'

In [10]: pion
Out[10]: <phasespace.phasespace.Particle at 0x22937232588>

I was really expecting to see the pion mass set in the constructor pion = Particle('pi+', 139.57018) with a simply pion.mass. Also, you implemented no __repr__. I really like these. In the Particle package we have <Particle: name="pi+", pdgid=211, mass=139.57061 ± 0.00024 MeV> Why would you not set the __repr__ to something like <phasesepace.GenParticle: name="pi+", mass=139.57018 MeV> ? You could even add in the representation a has_children=True/False ...

That's all for now.

[eduardo-rodrigues]

I forgot to say: personally I prefer generate to generate_decay. Why did you change?

[apuignav]

GenParticle is not a bad name, thanks! @mayou36 what do you think?

Some answers to your comments:

• There is no mass property because the mass can be a function, so it's something we didn't want accessible. There is get_mass to handle the complex situation. Does this make sense?
• Agreed on `repr, #25 tries to address this.
• I think generate_decay is a remainder of the name reorganisation we were planning, but I agree it doesn't make sense anymore. I think I would move to generate.

[eduardo-rodrigues]

All understood/fair. Thanks @apuignav.

[jonas-eschle]

Yes, I think GenParticle would be fine with me, thanks for the suggestion!

on generate: we had it before (I think). The point is that calling it generate means having two methods with the same name but different API. For a package containing about 4-5 methods in total (for usage), this seems not an optimal choice. But it may points to something different: what if we remake it to decay, which doesn't generate but just builds a decay and returns the composed particles. This means that we would have four names with a clear distinction.

• P.generate generates numpy
• P.generate_tensor generates a Tensor
• phsp.decay composes a decay in a functional manner
• Particle can be used for an object-oriented and more complicated decay composition

Makes it clearer. And adds only one line if using phsp.decay instead of phsp.generate_decay. Actually, even a shorter phsp.decay(...).generate(...) would work, splitting the building and the generation clearer.

[eduardo-rodrigues]

Glad my feedback is triggering a useful discussion :-).

I have not checked all code but what you mention makes sense. Just one detail: I reckon it is best to stick to one name for the actual generation, whatever the class/module is. That's what APIs do. Imagine you would compare different classes and packages for toy generation. One would have phasespace.generate, ToyGeneratorModule.generate, SpecialModel.generate, etc.

As for phsp.decay(...).generate(...): if the first method builds a decay chain and does no generation, then why not make it explicit with phsp.build_decay(...).generate(...)? I'm not a big fan of minimalistic names that loose info. Said in other terms, I do not mind writing a few extra characters if that makes the code clearer.

Just my 2 cents.

[apuignav]

I am a bit against building the decay as an intermediate step, as using the chain with the Particle class is trivial. I think a function that allows you to do quick generation may be useful, but we could drop it altogether (doesn't add a lot of functionality, honestly). Otherwise, we need to find a good, explicit name for it.

[apuignav]

@eduardo-rodrigues @mayou36 I've been thinking about this and we could rename phasespace.generate to phasespace.generate_simple. What do you think?

[eduardo-rodrigues]

Hmm, to be honest, I don't like it that much. To me the issue is that you have 2 ways to generate events, and it's tricky to find the right API. Imagine for a second that you had a ToyGenerator class. You could have TFPhspGenerator.generate(...) for the simple case (masses provided) and TFPhspGenerator.from_decay(...).generate(...) for the second case when Particle instances are set up (this latter from_decay is a class method). I realise this is a bit more code, but sometimes that's justified. At least this unifies the API. Maybe worth considering?

[apuignav]

I like this idea, but there's the problem that currently in generate we do more than simply assign masses: we assign names and afterwards flatten the output.

So I would suggest we have phasespace.nbody_decay(top_mass, children_masses, top_name=None, children_names=None) which creates the GenParticle structure. If top_name or children_names are not given, we assign them as top and p_{i} respectively. What do you think about this?

[eduardo-rodrigues]

Seems fine to me.