impy-project / chromo

Hadronic Interaction Model interface in PYthon
Other
30 stars 7 forks source link

make parents index zero-based #166

Closed jncots closed 1 year ago

jncots commented 1 year ago

This is PR to resolve issue #100

jncots commented 1 year ago

I changed parents -> mothers and children->daughters.

# Fix special cases
if mothers is not None:
    mothers[mothers == -2] = -1

if daughters is not None:
    daughters[daughters == -2] = -1

I am not sure about this snippet.

parents=ev.mothers + 1 if ev.mothers is not None else None,
children=ev.daughters + 1 if ev.daughters is not None else None,
HDembinski commented 1 year ago

The failing tests may come from the rather complex rules how ranges of particle indices can be defined. There are probably some special cases that you need to handle.

jncots commented 1 year ago

I tested 2 variants and parent = np.maximum(-1, parent - 1) is faster (~3 times) than 2 separate numpy operations. It is, probably, because numpy should not return to python level. So I changed the code to parent = np.maximum(-1, parent - 1).

jncots commented 1 year ago

It seems that tests are fixed. Most of the problems were because of _select_mothers function. The tests passes, but the function should be checked on whether the initial logic is preserved.

HDembinski commented 1 year ago

Ok, I will have a look.

HDembinski commented 1 year ago

It seems that tests are fixed. Most of the problems were because of _select_mothers function. The tests passes, but the function should be checked on whether the initial logic is preserved.

It looks ok, but rather than checking the implementation manually, we should have tests that cover all the common and special cases.

jncots commented 1 year ago

It looks ok, but rather than checking the implementation manually, we should have tests that cover all the common and special cases.

_select_mothers function seems to be used in many tests. I don't know whether we need additional one.

jncots commented 1 year ago

Sometime in the far future we can deprecate mothers and daughters and move back to the PC versions parents and children

Sure, parents and children are better.