ctapipe Bi-Weekly Digest — Activity since 2025-04-29

» cta-observatory/ctapipe | cta-observatory/ctapipe

• cta-observatory/ctapipe repo issues [back to top]

  • pyirf >=0.12.0 needs to be required, otherwise ctapipe-optimize-event-selection breaks #2723 [simonleeADL] bug

    Describe the bug When I installed the latest ctapipe, I had pyirf 0.11.0 already cached. This however caused an...

  • Make some dependencies optional #2638 [maxnoe]

    To enable some more minimal installations of ctapipe, e.g. for ACADA SAG, we should make at least the following...

    • GernotMaier: Stumbled over it while checking the size of a docker container with ctapipe installed: numba...

  • Add tool to export astropy cache to specific location #2748 [maxnoe] enhancement

    Please describe the use case that requires this feature. For working offline and on clusters, we should have a tool...

• cta-observatory/ctapipe repo pull_requests [back to top]

  • Add a StereoDispCombiner #2731 [Hckjs]

    This is basically the StereoCombiner already implemented here in magic-cta-pipe. It combines the mono DISP...

  • Remove one level of indirection from DVR classes #2750 [maxnoe]

    • kosack: Looks good, just need to include the tel_id and selected_gain_channel in the tests and other...

    • Hckjs: One could separate the actual reduction algorithm in the __call__-function to its own function...

    • kosack

    • Hckjs

    • maxnoe: One could separate the actual reduction algorithm in the call-function to its own function and...

  • Use actual muon PDF for toy model ring #2751 [maxnoe]

    @burmist-git This implements the expected muon intensity distribution for the asymmetry of the toy model.

    • burmist-git: This does not include the muon inclination.

  • Muon fit improvement #2736 [burmist-git]

    The muon ring fitter needs to be improved. The existing toy tests for muons are overly simplified. Realistic...

    • mexanick: Thanks for this update. I have a few questions and suggestions inline. Besides, I noticed that...

      • mexanick: Please use numpydoc convention and detail the parameters. For example: ...

        • burmist-git: Chat GPT ... I did same.

      • mexanick: Why nanmin and nanmax? Do we expect to have NaNs in the var? Also, if yes, what if we have var =...

        • mexanick: Suggested change min_var =...

        • mexanick: @burmist-git Please implement this suggestion

        • mexanick: @burmist-git please do not resolve the threads without addressing the concern.

        • maxnoe: Changing to a warning above makes the points by @mexanick important again

        • mexanick: I still can't get how in normal operation we can get NaNs there. The var is an array of...

        • burmist-git: f a log-like probability density function (PDF) receives a value like -1, it returns NaN...

        • burmist-git: If a log-like probability density function (PDF) receives a value such as -1, it returns NaN due...

        • burmist-git: Change raise Warning to warnings.warn in the last commit. During the pdf generation one can use...

      • mexanick: As we discussed offline, please add tests covering also LST geometry, and perhaps, nectarcam...

        • maxnoe: @burmist-git Has the comment here regarding test fragility been addressed?

        • burmist-git: @maxnoe yes and the origin of these fragility was simple we were very close to the boundary and...

    • maxnoe: please use astropy units here, i.e. 0 * u.deg

      • burmist-git: Ups ... Done

    • maxnoe: As I wrote before, this is a changelog entry. It describes what has already happened.

      • burmist-git: So this was to the current changes ... Ok. Done.

    • maxnoe: It does not make sense to use 1.1 in whatever unit was passed in. 1.1 only makes sense in degrees.

      • burmist-git: Correct, that is how it is done now : if original_unit == u.deg: r_initial = 1.1 *...

    • maxnoe: This will fail for the default argument, as it is still 0 not 0 * u.deg

      • mexanick: also, if provided with proper units, the to_value conversion is not needed.

    • mexanick: Suggested change max_var =...

      • mexanick: @burmist-git Please implement this suggestion

      • burmist-git: If a log-like probability density function (PDF) receives a value like -1, it returns NaN...

      • burmist-git: If a log-like probability density function (PDF) receives a value such as -1, it returns NaN due...

      • burmist-git: Change raise Warning to warnings.warn in the last commit. During the pdf generation one can use...

      • maxnoe: If a log-like probability density function (PDF) receives a value such as -1, it returns NaN due...

      • burmist-git: I can repeat my answer again : Change raise Warning to warnings.warn in the last commit. During...

    • maxnoe: Was the last commit that added another method here intentional? I'd suggest to revert it, and...

    • mexanick: I think the default should be False (correct error calculation) Suggested...

      • maxnoe: Why would we ever want dummy errors?

      • burmist-git: There can be two reasons : 1) safe the cpu time. 2) We are not using the errors for the moment...

    • maxnoe: @burmist-git I am confused. You said this PR is ready for review / merging, but now you keep...

    • mexanick: @burmist-git I am confused. You said this PR is ready for review / merging, but now you keep...

    • burmist-git: @burmist-git I am confused. You said this PR is ready for review / merging, but now you keep...

    • maxnoe: These are magic numbers that don't seem to have any meaning. Where do they come from? Why do we...

      • burmist-git: This one is done now - I set ti to nan.

      • burmist-git: I rename the flag to nan_errors_flag

    • maxnoe: We have dummy errors and naive errors?

      • burmist-git: The optimum circular fit to weighted data in multi-dimensional space (Chaudhuri, 1993), used in...

      • burmist-git: If you have a different function name in mind, please let me know.

    • mexanick: I'd remove the print statements. If you want to check/test something, use assert, otherwise this...

      • burmist-git: I removed the print of the method name. The -s option enables the printouts.

    • mexanick: Suggested change - the function may...

    • mexanick: Overall, LGTM. A few minor comments inline.

    • mexanick: @burmist-git docs are giving an...

    • maxnoe: This is still an exception, since you raise it. To warn, you need warnings.warn and you can pass...

      • burmist-git: The linear_pdf function is not used frequently and is not a standard PDF function. It is written...

    • mexanick: asymmetry_orientation_angle_deg=0 * u.rad, looks weird semantically. ...

    • mexanick: to_value conversion is not need here

    • mexanick: Please remove all the print statements

    • maxnoe: DESY infrastructure is down for critical repairs following the power outage. It's planned to be...

    • burmist-git: @burmist-git docs are giving an...

    • mexanick: @burmist-git docs are giving an...

    • burmist-git: @burmist-git docs are giving an...

    • maxnoe: That's not what I meant. You should include documentation of the new methods you introduced here.

    • maxnoe: Just use iminuit instead of nitpick ignoring it. If the link doesn't work, there is no point in...

      • burmist-git: ?? I did not get what do you mean.

      • burmist-git: I hope done

    • maxnoe: Please explain why we would want to switch off error computation. In case we really do want...

      • burmist-git: Ok. I remove them.

    • maxnoe: Why wouldn't we use 1.1 * u.deg if the input unit happens to be radians?

    • maxnoe: Why simplified? What would it take to make it properly?

      • maxnoe: Where does this computation come from? Is there a reference?

      • burmist-git: Naive (simplified) error calculator for circular data with weights. In this naive approach, we...

      • burmist-git: https://private-user-images.githubusercontent.com/55189646/439384043-000624bd-b18f-499c-8fd6-26b8...

      • burmist-git: pulls are ok.

    • mexanick

    • mexanick: lines 153-156 can be simplified to this: Suggested change ...

    • burmist-git: @maxnoe @kosack We're now confident there are no major additions needed. You're very welcome to...

    • maxnoe: Looks mostly good, some remaining questions and style comments

      • maxnoe: Missing newline between sections in the docstring

      • maxnoe: I asked in another review if there would be a reason to switch off the error computation and...

        • burmist-git: The performance is not degraded, and the errors can be used—and will be used—later in this step....

        • maxnoe: I was talking about runtime. The uncertainty computation perform many more operations than the...

        • burmist-git: yes, but it is ok for the CPU. In any case we will use it this error calculation.

        • maxnoe: How did you make this decision? How can performing unneeded computations "be ok for the...

        • burmist-git: @maxnoe .... Ok i putting back the nans.

      • maxnoe: The new uncertainties here are just ignored. They should also be tested.

        • burmist-git: Yes, they should be tested, and we need to include the pull test, as shown in the plot I shared...

        • maxnoe: I think it's reasonable to proceed with the merge request and include these new tests in a...

      • maxnoe: This is essentially a big switch that reacting on the parametrization of the test. But why not...

        • burmist-git: Done. But not for the : prod5_lst, prod5_mst_flashcam, prod5_mst_nectarcam, ...

        • mexanick: The patch I suggested in my another review shows how to parametrize the fixtures.

        • burmist-git: @mexanick It is not well readable ... Additionally we need to pass different parameters for...

        • burmist-git: @maxnoe @maxnoe Now the code have smell since we pass 14 parameters (max should be 13) ... Let...

        • maxnoe: You are still passing all the telescope descriptions as arguments here instead of just passing a...

        • burmist-git: the fixture is not a usual argument... I really do not understand what do you mean. If you have...

        • burmist-git: you assume that you can pass the fixture with camera as an argument ... no you can not.

        • mexanick: @burmist-git this works: import astropy.units as u import numpy as np import pytest from...

        • burmist-git: Done

    • mexanick

    • burmist-git: @maxnoe #2736 (comment) However, when applying the inverse function method, you may encounter...

    • maxnoe: How can this happen?

      • burmist-git: only if var is nan and in case of inverse function method usage for more complex pdf...

      • maxnoe: in case of inverse function method usage for more complex pdf calculation. Where does this...

      • burmist-git: More complex pdf calculation happen in possible future :-) only. The toy model now is close to...

      • maxnoe: But why would you fit the toy model to real muon rings?

      • burmist-git: Actually the methods will do anything the toy will provide. we decided to test them with "real"...

      • burmist-git: And on more thing: the image intercity in current toy model is actually a function of its size,...

      • burmist-git: Or you asked me about the more complex pdf calculation - this may not happen. The whole story...

      • maxnoe: The pdf(NAN) = 0.0. I follow this logic only when I did the function implementation. I don't...

      • maxnoe: Which makes me think: shouldn't the linear PDF be defined in terms of center and radius (e.g....

      • burmist-git: The linear pdf is linear pdf you provide the shift and slope and its provide you the PDF minimum...

      • maxnoe: Yes, the slope is provided. However, the minimum and maximum values of the input variable still...

      • maxnoe: This could be fixed by replacing this linear distribution by something I asked in one of my...

      • maxnoe: More realistic would be using the actual PDF for muon rings, in terms of the impact parameter...

      • maxnoe: I opened #2751 targeting this PR with the last option

      • burmist-git: This about actual PDF for muon rings. @maxnoe This does not include the muon inclination w.r.t...

      • burmist-git: The first point : However, the minimum and maximum values of the input variable still...

      • maxnoe: This does not include the muon inclination w.r.t optical axis ... The intensity is not really a...

      • maxnoe: We have the full model in the intensity fitter, so I think there is no point in making the toy...

      • burmist-git: :-) still think it was doing the reasonable job the linear_pdf.

    • maxnoe

    • maxnoe: The test that fails in the CI passes locally for me, so it seems the comment regarding fragility...

    • burmist-git: @maxnoe can i see the logs ?

    • mexanick: @maxnoe @burmist-git this is the relevant portion of the log: FAILED...

    • burmist-git: @maxnoe probably the intensity have to be returned since new function (the muon pdf vs impact...

    • maxnoe: The phi_pdf does not integrate to 1. Looks like a factor 2π is missing: import astropy.units as...

    • burmist-git: Yes this ring can have problems :-).

    • burmist-git: So the intensity now is a strong function of rho. This has been changed and taken into account.

    • burmist-git: @maxnoe this effectively is the change of normalization. You probably got even more errors now....

    • maxnoe: The psi PDF should now be normalized correctly for all cases. I adjusted the image brightness...

created at 2025-05-13 06:33 by dinghy 1.4.1.