Muon fit chord length fix

[burmist-git]

Modified the chord_length function to fix a bug. It now filters out non-physical solutions for chodr vs phi.
Updated the test for chord_length calculation in the muon intensity fitter. It now checks that no non-physical solutions are returned (more details in here : issue #2760).

[burmist-git]

#2760 (comment)

[mexanick]

according to sonar, these two lines are not covered by tests:

        if np.abs(phi) < np.arcsin(1.0 / rho):
            chord = 2 * radius * np.sqrt(chord)

Please check and ensure this is covered by proper tests.

[burmist-git]

These two lines are fully covered by this test. If you remove this check, the test will fail. Please have a look—I removed this check, and the test failed on my laptop.

[mexanick]

your test verifies the part when condition np.abs(phi) < np.arcsin(1.0 / rho) is not met, but looks like it doesn't check when it is met.

[burmist-git]

Okay, values where rho is greater than 1 and phi is 0 can be included

[mexanick]

You have four cases in the function: no chord, a muon outside of the circle, and two cases for muon inside the circle. All of them have to be tested. Also, border cases (e.g. a hit straight in the center) and at the tangentially with φ=π/2 could be interesting to test. as well.

[burmist-git]

Yes and now we have 4 tests :

Two inside and two outside

[mexanick]

I'm not sure why the sonar coverage is ill-computed... @kosack @maxnoe any idea? Could it be a consequence of a transfer to the new sonar instance?

@burmist-git I compiled the documentation and discovered that no sphinx doc is produced for the the module-level functions in the ctapipe.image.muon.intensity_fitter module. Please fix this by:

• add in __all__ all the functions that aren't private
• Update module-level docstring (I believe it is outdated)
• perhaps something else I missed - please verify that the doc assembles correctly

Also I noticed the a couple of warnings like the following one, all in the ctapipe.image.muon.ring_fitter module:

UserWarning: potentially wrong underline length... 
Returns 
-------- in 
Perform a circle fit to ``image`` with the chosen ``fit_method``.
... in the docstring of __call__ in /ctapipe/src/ctapipe/image/muon/ring_fitter.py.

Please fix them too.

[maxnoe]

@burmist-git sorry, I discussed with @mexanick offline and we agreed that the docs should stay and it's the correct workaround for an issue with numba: numba/numba#5755

Could you revert the last two commits? This will also fix the ci

[burmist-git]

@maxnoe Sure, I can do that. The only remaining question is the directory of chord_length.rst. It was here: docs/api-reference/image/chord_length.rst, but it probably should be here: docs/api-reference/image/muon/chord_length.rst.

[maxnoe]

The only remaining question is the directory

There currently is no muon subdirectory in the documentation, everything is flat in image in files, so it's fine. Maybe call the file muon_chord_length.rst to make it clearer.

[maxnoe]

Ah, sorry, misread the commits.

[maxnoe]

You need to change the entry in the toc in docs/api-reference/image/muon.rst to take the changed name into account.

Edit: done now

[burmist-git]

change the name of the rts file

[burmist-git]

@maxnoe The only valid option for me was to review changes, could not apply them.
All is done than :

[ctao-dpps-sonarqube]

• 53.30% Coverage on New Code (is less than 80.00%)

Analysis Details

0 Issues

• 0 Bugs
• 0 Vulnerabilities
• 0 Code Smells

Coverage and Duplications

• 53.30% Coverage (94.30% Estimated after merge)
• 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube