Add Corsika7ID (+converter) and Particle.from_nucleus() #426
Conversation
- added the class Corsika7ID (adapted from the class Geant3ID) - some tests for the class - added the script that generated the conversion table - Corsika7ID has some functionality to see if conversion to PDGID is sensible: Check if the Corsika7ID relates to a particle via .is_particle(), get the human readable name via .name()
- This allows easy conversion from the corsika7 output to Corsika7ID
More structured order
Codecov ReportBase: 90.79% // Head: 89.94% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #426 +/- ##
==========================================
- Coverage 90.79% 89.94% -0.86%
==========================================
Files 29 32 +3
Lines 1326 1412 +86
==========================================
+ Hits 1204 1270 +66
- Misses 122 142 +20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Hello @The-Ludwig, thank you for the interest in the package and this contribution. I will look at it asap. Certainly we are always keen on including more links to other MC IDs ... I very much prefer scoped MRs. Also it is often better to start some discussion with the maintainers before actual commits, to streamline the route to follow. Can you move your discussion on ForeignParticleID to an actual separate "issue"? As said I will look asap. This being said, why do you say there was a need for something like Particle.from_nucleus()? Could you not make the nucleus Particle with the finditer() method given that it provides a powerful "filtering" with lambda functions? To be seen if the lack of way was not more like a lack of an example. |
|
Hej @eduardo-rodrigues, thanks for the reply! Yes, I realized this PR went a little all over the place... Also, there will be certainly things which need improving (more tests, some weird variable naming, etc...). Please let me know what I can do to improve it. Sure, I'll open an Issue. True, you can always find a nucleus with for example |
|
Hello @The-Ludwig, I'm being a bit slow. No sure whether I mentioned that I was a bit loaded recently. In any case I will review this PR this week. Indeed really nice to see this extension in, which will I hope be of interest to astroparticle colleagues ... BTW, are you in astroparticles/cosmics, hence the need for Corsika? I know, I'm being curious ;-).
Definitely the way to go when working in community software 👍 ! |
|
Hi @eduardo-rodrigues, |
Thanks a lot. Am looking in batches in between meetings ... |
This is probably acceptable as one can still compare by hand the newly produced file, and Git kind of does it for you, actually. This being said, I did not try but one could still keep the test navigating the directories up one (relative) level. Maybe it is a bit shaky, though. Let's leave it as-is now. |
|
Hmm, those imports are a bit annoying if they provoke circular dependencies. Are you sure of those being circular? Did not check carefully enough but did not see the "circle". with a line comment - something like If you try and fix locally and then commit? The other CI / Check failures are weird. |
I checked again and if I do a top-level import I get I'll suppress the warning. |
|
Have you tried with a relative import? I think you are doing absolute imports there. |
I used |
|
Oh, see it. OK. |
|
Hello @The-Ludwig, as you can see from https://app.codecov.io/gh/scikit-hep/particle/tree/master/src/particle/corsika, the present test coverage for the module you added is not fantastic. In the new year I will try and continue some recent commits to improve coverage, and wanted to ask you whether you would be willing to take care of improving coverage for the corsika module? Let me know whether January is viable to you. Advance thanks and season greetings! |
|
Happy New Year @The-Ludwig! Let me know what you think of the above ... Many thanks. |
|
Happy new year @eduardo-rodrigues ! :) I've not been online much in the past weeks, sorry! |
|
That's great news 👍 ! Nothing special. Have a look at the present coverage at https://app.codecov.io/gh/scikit-hep/particle/blob/master/src/particle/corsika/corsika7id.py or locally and check also similar test files, as it can help. Advance thanks! |
I didn't find a user friendly method to convert a Corsika 7 ID to standard PDGID, so I tried to implement it here.
While implementing it, I noticed there was no function to easily generate nucleus particles, so I added function
Particle.from_nucleus(), which first generates the right PDGID out of the nucleus properties (according to this pdf) and then usesParticle.from_pdgid.For the implementation of
Corsika7IDI tried to copy the style of the other IDs. I hope it is okay-ish like this.Next comment moved to Issue #427.
While working on this I was wondering: Would it be a good idea to implement an abstract base class like
to let the other IDs present in this library inherit from this. Then we could have a method like
Particle.from_foreign_id(id: ForeignParticleID).