feat: add overdetermined morphing

[wangzhe04]

This pull request implements the following changes to morphing.py:

1. Add the overdetermined morphing functionality which allows users to input more input as many basis points as possible.
2. Add the additional input format that allows users to specify coupling for production, decay, and both separately
3. Add an tutorial Jupyter Notebook for using morphing.py in the directory examples/morphing_example.ipynb
4. Add additional tests for testing the implemented features in tests/test_morphing.py

[alexander-held]

Thanks for opening this! You can add the sympy dependency to setup.cfg here:

madminer/setup.cfg

Lines 28 to 36 in dd223f8

	install_requires =
	h5py
	matplotlib>=2.0.0
	numpy>=1.20.0
	particle>=0.16.0
	scipy>=1.0.0
	torch>=1.0.0
	uproot3>=3.14.1
	vector>=0.8.4

. That should cause it to be picked up for the tests in continuous integration. The tests are probably going to fail without updates, so we'll also have to look into updating those and testing the new functionality.

There are currently a lot of empty lines getting removed, is this on purpose? It might be best to avoid this in this PR and do it separately in another one if it is a matter of just formatting.

Can you reproduce the issue you saw with particle? And do you mean utils/particle.py or some other file? If there is a conflict that can appear, it would be good to find a way to avoid this as well.

[wangzhe04]

Yes, I can reproduce the error, this is the error message i got after running a new runner file in the docker environement.

Code:
import morphing as m
if name=="main":
a = m.PhysicsMorpher(parameter_max_power=[2,2])

Error:
root@976930b62f62:/madminer/madminer/madminer/utils# /bin/python3 /madminer/madminer/madminer/utils/non_runner.py
Traceback (most recent call last):
File "/madminer/madminer/madminer/utils/non_runner.py", line 1, in
import morphing as m
File "/madminer/madminer/madminer/utils/morphing.py", line 9, in
from madminer.models import Benchmark
File "/usr/local/lib/python3.8/dist-packages/madminer/init.py", line 4, in
from .delphes import DelphesReader
File "/usr/local/lib/python3.8/dist-packages/madminer/delphes/init.py", line 1, in
from .delphes_reader import DelphesReader
File "/usr/local/lib/python3.8/dist-packages/madminer/delphes/delphes_reader.py", line 10, in
from madminer.utils.interfaces.delphes_root import parse_delphes_root_file
File "/usr/local/lib/python3.8/dist-packages/madminer/utils/interfaces/delphes_root.py", line 11, in
from particle import Particle
File "/madminer/madminer/madminer/utils/particle.py", line 4, in
from particle import Particle
ImportError: cannot import name 'Particle' from partially initialized module 'particle' (most likely due to a circular import) (/madminer/madminer/madminer/utils/particle.py)

I think it can be solved by changing the utils/particle.py to a new name such as utils/particle1.py

Also I realized that the morphing weights function and probably the gradient funciton also need to change due to refactor. I haven't change these functions yet, I'll try finish these tomorrow.

[alexander-held]

The issue you see is due to running from within the madminer library, you can resolve it by running your code from elsewhere, then

from madminer.utils import morphing as m

if __name__ == "main":
    a = m.PhysicsMorpher(parameter_max_power=[2, 2])

works fine. In order to be able to import the code in this fashion, while still being able to change the morphing module and see the effects, you can do an editable install e.g. via pip install -e . from within the madminer repository.

[Sinclert]

👋🏻 Hi @wangzhe04

Thank you for your contributions, they are highly appreciated :)

Tuning in to ask for tests covering the modified PhysicsMorpher functions before marking this PR ready to review (as of now: find_components, set_basis and calculate_morphing_matrix). The test suite is currently very limited, and increasing it will make development sustainable over time.

[wangzhe04]

👋🏻 Hi @wangzhe04

Thank you for your contributions, they are highly appreciated :)

Tuning in to ask for tests covering the modified PhysicsMorpher functions before marking this PR ready to review (as of now: find_components, set_basis and calculate_morphing_matrix). The test suite is currently very limited, and increasing it will make development sustainable over time.

Thank you! Thank you for your contributions as well!
I'll surely include some testings for these modified parts!

[matthewfeickert]

@alexander-held suggested in a meeting today that @wangzhe04 write some tests for the project that target the areas he's hoping to extend for his IRIS-HEP Fellow project in another PR. As @Sinclert has already pointed out, this will give the nice effect of both adding coverage in addition to giving a nice method to check to make sure that the changes @wangzhe04 makes are doing what are expected. 🙂

@wangzhe04 if you wanted to outline some of the ideas for this project in a GitHub Issue that might make it easier to step through some other parts later on in the review.

[wangzhe04]

@alexander-held suggested in a meeting today that @wangzhe04 write some tests for the project that target the areas he's hoping to extend for his IRIS-HEP Fellow project in another PR. As @Sinclert has already pointed out, this will give the nice effect of both adding coverage in addition to giving a nice method to check to make sure that the changes @wangzhe04 makes are doing what are expected. 🙂

@wangzhe04 if you wanted to outline some of the ideas for this project in a GitHub Issue that might make it easier to step through some other parts later on in the review.

Sure, I'll look into creating a Github Issue tomorrow, thanks!

[wangzhe04]

Changed to pull request#516

[Sinclert]

Hey @wangzhe04 .

Thanks again for all the effort you are putting into this. Having tests for such a big change is crucial :)

I am curious though, why are you moving changes to new branches, opening new PRs while closing previous ones? I am afraid conversation / context may be lost because of this dance.

[wangzhe04]

Hey @wangzhe04 .

Thanks again for all the effort you are putting into this. Having tests for such a big change is crucial :)

I am curious though, why are you moving changes to new branches, opening new PRs while closing previous ones? I am afraid conversation / context may be lost because of this dance.

Oh, I saw the changes made in this PR are also showing in the new PR so I thought there is no need to keep this one. I'll reopen this one. Thank you for pointing out.

[alexander-held]

I think this was a misunderstanding about the strategy, sorry I was probably not very clear about this. My proposal is getting tests for the existing functionality in first (which we can do via #516) and then use this PR to add the new functionality + expand the tests to cover the new additions.

[Sinclert]

I noticed the PR adding Morphing tests (#516) was directly merged from its draft state, so I decided to go ahead and provide feedback for this one despite its current state.

Once again, thanks for your contributions :)

[alexander-held]

I noticed the PR adding Morphing tests (#516) was directly merged from its draft state, so I decided to go ahead and provide feedback for this one despite its current state.

I unmarked from draft before looking over it again and decided to go ahead since it was a straightforward PR in my view. If you prefer, I am happy to always wait for your explicit approval instead @Sinclert. Either way, this PR is bigger and we were planning on sending a request once all changes are done (which should be within the next day or two I think, thanks for already having a look!).

[wangzhe04]

In the new commit I included a function in the example notebook that it can check the minimum number of basis points input requires. Should we include that in the morphing.py as well? It's a simple function and independent function so probably can be added the easily

[alexander-held]

That looks very useful to me! I think this would be a great addition to morphing.py in a new function (with a dedicated test for just this piece of functionality).

[wangzhe04]

I added get_min_basis function and the tests and also updated the morphing_example.ipynb to use the built in one. There was a mistake and some format issues in the test_morphing.py I first commited but it should be fixed now.

[Sinclert]

This is probably the last batch of requested changes! I will approve this PR as soon as they are addressed.

[Sinclert]

LGTM as it is! What do you think? @matthewfeickert @alexander-held

[alexander-held]

This looks good to me, I have no other comments. Thank you very much for getting this ready and all the interactions on the PR!

[matthewfeickert]

Taking a very quick look at this before lunch I think this is fine (added tests are very nice!). If I have any other comments I will open them up as separate Issues that can get resolved after this is merged. Thanks for the contributions @wangzhe04 and for @Sinclert and @alexander-held for the guidance here. 👍

[alexander-held]

Thanks everyone! I'll leave it to @Sinclert to hit merge. For future reference, @wangzhe04 will present this work done as an IRIS-HEP fellow also at a meeting tomorrow: https://indico.cern.ch/event/1195271/.