Add support for parsing decay descriptors - #573
Conversation
|
Hi @admorris, I am not forgetting to check this. It's just that I have been working on urgent and important suff. Will get back to you very soon. |
| return cls(mother, decay_modes) | ||
|
|
||
| @classmethod | ||
| def from_string( |
There was a problem hiding this comment.
At some point it would make sense to "synchronise" this from_string function with the existing to_string one, since they should effectively be the "mirror of each other". Else one would name this function to from_descriptor. WDYT?
There was a problem hiding this comment.
The pytest test_from_string_to_string demonstrates they mirror eachother in the specific case of using the default grammar.
What if this function is renamed from_descriptor, then from_string just invokes from_descriptor with the default grammar?
There was a problem hiding this comment.
Yes, that sounds good to me.
eduardo-rodrigues
left a comment
There was a problem hiding this comment.
Thank you so much for this, @admorris! It's a really nice enhancement 👍.
I left a few little suggestions but this is looking excellent anway.
I am well aware of the limitation you point out. It does annoy me.
|
Hey @admorris, let me know your thoughts on this enhancement and whether you would prefer to have a follow-up for the limitations and some of the matters discussed above. BTW, for the issue with parsing parenthesis for particle names. One thing that differentiates parentheses for particle names wrt parenthesis denoting decays is that the latter always contain an arrow inside, so between ( and ). |
|
Hey. So aside the trivial comment above, I think there are only 2 things to get sorted:$
|
|
Hi @admorris, you may have seen the major improvements made to the package recently, thanks to @henryiii. We are moving towards a 1.0 release ... Let me know if you will be able to pick up the work here after a rebase. FYI you can now do the following: |
|
Very good! I will have a look |
Co-authored-by: Eduardo Rodrigues <eduardo.rodrigues@cern.ch>
2943a59 to
c8eb6de
Compare
|
Whenever you are ready, I can do an Opus 4.8 or GPT 5.5 review of the PR if that sounds helpful. (if you are using one of those, or already ran a review with one, let me know and I'll do the other one, otherwise I'll do Opus because I've got higher limits on that) |
Hello @henryiii, I will get back to this PR now that version 1.0 is out. Before I do a "physics" review, if you can then run an Opus or similar review, that will be great. Thank you. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #573 +/- ##
==========================================
- Coverage 97.62% 96.38% -1.25%
==========================================
Files 6 6
Lines 1222 1299 +77
==========================================
+ Hits 1193 1252 +59
- Misses 29 47 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Hi @henryiii, a lot has been happening recently. I would like to get back to this now that the other developments are available in the latest 1.x releases. Very happy, then, to have an AI review from you. Let me know, thanks. |
|
Claude Fable standard review: 🤖 AI text below 🤖 Review of PR #573 — Add support for parsing decay descriptorsOverviewAdds The design fits the codebase well — same Lark-grammar-in- Issues1. Error contract is leaky for same-name sub-decays (verified by running the PR code). The docstring promises
Suggest wrapping the 2. Misleading error for a broken custom grammar ( 3. Parser is rebuilt on every call. LALR table construction runs per 4. RST underline too short ( 5. Missing Test coverageThe positive-path tests are good — repurposing the existing formatter examples for round-trip tests is exactly right. Gaps:
Minor / style
VerdictSolid, well-scoped feature that follows the project's grammar-driven architecture, and the grammar itself held up to every edge case I threw at it. The main asks before merge: tighten the error contract for same-name sub-decays (issue 1 — the actual behavior differs from what the PR description claims, in both directions), fix the misleading custom-grammar error path, add the missing negative/edge tests, and the changelog entry plus one-character RST fix. |
|
Super, thank you much, @henryiii. There's now some bits of work to do :). |
| "D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)", | ||
| "D*+ -> (D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)) pi+", | ||
| "B0 -> (D- -> K+ pi- pi-) (tau+ -> anti-nu_tau pi+ pi+ pi-) nu_tau", | ||
| "B_s0 -> (phi -> K+ K-) (phi' -> pi+ pi- pi0)", |
There was a problem hiding this comment.
Add here "B_s0 -> (phi -> K+ K-) (phi -> K+ K-)" as per my comment above on the limitation being a thing of the past.
|
Hello @admorris, cc @henryiii. The review is very good. There are things that we could leave for a follow-up enhancement. Other small things like better coverage would be useful, though. If you're really low on time let me know and I can try and step in in a follow-up - would avoid trying to work on your fork, which would require a sync with main. |
| "D*+ -> (D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)) pi+", | ||
| "B0 -> (D- -> K+ pi- pi-) (tau+ -> anti-nu_tau pi+ pi+ pi-) nu_tau", | ||
| "B_s0 -> (phi -> K+ K-) (phi' -> pi+ pi- pi0)", | ||
| ], |
There was a problem hiding this comment.
| ], | |
| "B_s0 -> (phi -> K+ K-) (phi -> K+ K-)", | |
| ], |
| @pytest.mark.parametrize( | ||
| "descriptor", | ||
| [ | ||
| "D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)", |
There was a problem hiding this comment.
| "D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)", | |
| "D0 -> K- pi+", | |
| "D0 -> (K_S0 -> pi+ pi-) (pi0 -> gamma gamma)", |
Implements the rest of #200
Added
DecayChain.from_stringmethodDecay descriptors are parsed with Lark. A
Transformerclass converts them intoDecayChainDictobjects, which are then used to initialiseDecayChainobjects.Custom descriptor formats can be used by pointing to another
.larkfile in an argument ofDecayChain.from_string. These pretty much only have the freedom to modifyARROW,LPARandRPAR. The rest of the structure is assumed by the Transformer. i.e. I did not find a way to support sub-decays written with the mother outside of braces likeA -> B (-> C D) EOne glaring limitation (which is inherent to
DecayChain/_build_decay_modes) is that sub-decays of identically named particles are not supported: e.g.."B_s0 -> (phi -> K+ K-) (phi -> K+ K-)"will result in an exception. This could possibly be handled by adding internal/hidden uniqueness when duplicates are encountered.