Dynamically supply list of decay models to grammar

[sognetic]

Hi @eduardo-rodrigues,

here's my second PR which replaces the hard-coded list of model names in the grammar with a placeholder which is filled dynamically with the list of models. I've marked the PR as a draft since there are a few points where I'm not sure what the way to handle things is. I'll comment on the relevant lines / changes in the PR and would be grateful for some input there.

All tests pass locally.

[eduardo-rodrigues]

Hey. A was not aware of this new(?) feature from Lark. What are the constraints to use it in terms of dependencies and Python version support?

TBH I quite like the idea that one can trivially see from the definition file what models are available, and extend if necessary, see https://github.com/scikit-hep/decaylanguage/blob/master/README.md#advanced-usage. What would be the new way to achieve

dfp = DecFileParser('my_decfile.dec')
dfp.load_grammar('path/to/my_updated_decfile.lark')
dfp.parse()
...

discussed there with your changes? Apologies that I'm really loaded and did not look carefully, and I could probably answer myself. I will do soon unless you make it easier for me :-).

[sognetic]

Hey. A was not aware of this new(?) feature from Lark. What are the constraints to use it in terms of dependencies and Python version support?

As far as I can tell, this (imo not very well-documented) feature has existed since version 0.7.7 of Lark, being added after discussion in this issue. I'm not sure what this means for Python version support but would argue that a feature that was added four years ago should be relatively safe. The tests with all different versions pass after all.

TBH I quite like the idea that one can trivially see from the definition file what models are available, [...]

I agree that having a full description in the grammar is nice but would argue that the simplicity of only having to maintain one list beats that. Having a hard-coded list of strings in the grammar also feels kind of weird to me, a change of the grammar should indicate a fundamental change of the language, not just an update of the list of allowed models. While the weird decfile language makes this list kind of necessary (I really tried to come up with a way around hard-coding the models but couldn't come up with one) it's not really desirable in my opinion.

[...] and extend if necessary, see https://github.com/scikit-hep/decaylanguage/blob/master/README.md#advanced-usage. What would be the new way to achieve

dfp = DecFileParser('my_decfile.dec')
dfp.load_grammar('path/to/my_updated_decfile.lark')
dfp.parse()
...

discussed there with your changes?

If path/to/my_updated_decfile.lark doesn't contain MODEL_NAME_PLACEHOLDER I think the example should still work and would just use the hard-coded list of models in the grammar, right? Additionally, we could add an optional argument to load_grammar() which could be used to supply an alternative list of models and the appropriate placeholder string.

Apologies that I'm really loaded and did not look carefully, and I could probably answer myself. I will do soon unless you make it easier for me :-).

No worries, it's not an urgent feature really but I thought I'd just put it in a draft so I don't forget this exists and to start some discussion on whether this is something you'd want in decaylanguage.

[eduardo-rodrigues]

Hello @sognetic, sorry for being a bit silent here. You may have seen that I have been very busy finalising the tests of the EvtGen models. I'm now basically done and you can see from https://github.com/scikit-hep/decaylanguage/pull/396 that some enhancements and fixes were very important to get in. I'm afraid you will need to rebase one last time, but for a good cause.

Actually, wait until next week to rebase as I will have another enhancement :-).

[eduardo-rodrigues]

I would suggest that we go ahead here with the removal and we then clean-up a bit. I see also that the notebook example needs an update ... Let me know what's your timeline, so that I adapt - I have a new PR to make to provide full coverage of Belle II models, so if you tell me you cannot work in the next few days I can make that MR and you then rebase on the full set of my PRs, else I will wait. Just let me know.

Thanks.

[sognetic]

Okay, I wasn't sure whether your proposed enhancements had already finished or not. Unfortunately I cannot work on this in the next seven days but will pick it up afterwards. I'll gladly merge in any changes improving Belle II coverage, thank you for doing this!

[eduardo-rodrigues]

No probs. Then I will try and finalise on the PR with Belle II support and you take it from there. Thanks.

[sognetic]

Hi @eduardo-rodrigues ,

I've now deprecated load_grammar, replaced it with an internal method and added a way to load additional models (with a small test for this feature). I've had to updated the Jupyter notebook to add a stripped-down version of the edit_terminals callback function (since that doesn't use dec.py which I didn't immediately notice).

[sognetic]

Hi, I hope this addresses all your comments. If not, please let me know and I'll try to fix it ASAP.

[sognetic]

Thank you for the great review and for adding the finishing touches!