ctapipe Bi-Weekly Digest — Activity since 2025-08-09

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

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

  • Energy resolution/dispersion with irf maker tool #2810 [BastienLacave]

    Hi, I am using ctapipe irf maker tool on CTLearn data, that is in the ctapipe format. First I optimize the cuts for...

    • maxnoe: Could you show both versions of the plots as well as the code you use to produce the plot using...

    • BastienLacave: Here is the code I use...

    • maxnoe: Is that combining all offsets together? the ctapipe tool computes in bins of FoV offset. What...

    • BastienLacave: I use the default FoV bins, so 1 bin [0, 5] deg

    • maxnoe: LST-1 MC gammas Are the observation ids / event ids unique? LST MC productions have issues here.

    • BastienLacave: I checked and indeed they are not unique... DL1A table: 3023659 total events, 200000 unique event_ids

    • BastienLacave: I realized that the pair (obs_id, event_id) is unique for each event

  • ctapipe-train-disp-reconstructor fails when using dl1 files processed with --camera-frame option #2812 [mescobargodoy] bug

    Describe the bug ctapipe-train-disp-reconstructor fails when using dl1 files processed with --camera-frame option as...

    • maxnoe: Thanks for the issue @mescobargodoy Please note that we deprecated support for doing...

  • The chord calculation has a bug #2804 [burmist-git] bug

    Describe the bug The chord calculation has a bug. It does not account for non-physical solutions in the chord...

  • The reflector’s hole radius is defined incorrectly #2807 [burmist-git] bug

    Describe the bug The reflector’s hole radius is defined incorrectly (default_value). To Reproduce The radius of the...

  • Enhancement of the muon analysis pipeline. #2760 [burmist-git] enhancement

    Please describe the use case that requires this feature. The muon analysis pipeline has recently been updated to...

  • Discussion: Metadata handling (citation.cff / codemeta.json) #2813 [vuillaut] enhancement

    Hi, I wanted to open a discussion around how metadata and citation information are currently handled in ctapipe (and...

    • maxnoe: Hi @vuillaut Thanks for bringing this up. I have nothing against adding these files, but I'd...

  • avoid duplicate docu? Or maybe cross-link? #942 [dneise]

    This is a question and maybe a reminder to myself for improving this. Not complaining at all. I chose the example of...

  • Dtypes in DL1 files #1411 [LukasNickel]

    I was trying to merge some dl1 files and ran into issues trying to append the content of one table to...

  • stage1 write-index-tables ignores exceptions / ClosedFileError #1607 [nbiederbeck] bug

    Running ctapipe-stage1 with --write-index-tables results in hundreds of the following block: Exception ignored in:...

    • maxnoe: This was fixed.

  • tels_with_trigger + allowed_tels in dl1 and simtel files #1372 [LukasNickel] bug question subarrays

    I am not 100% sure if this is a bug or a design decision I do not understand rightaway. If allowed_tels is given...

    • maxnoe: Fixed in #2136

  • AUTHORS not updated since two years #1699 [maxnoe]

  • Incomplete contributors list #2122 [vuillaut] maintenance

    Describe the bug It came to my attention that some code contributors do not appear in the list of contributors...

    • maxnoe: We now have the update of authors in the checklist for making a new release, I think it's fine.

  • Embracing type hints #1775 [maxnoe]

    Over the last couple of weeks I came to the conclusion that specifying type hints, especially for abstract class...

  • New method to reconstruct muon impact point #2815 [burmist-git] enhancement

    We need to implement a new method to reconstruct the impact point. With the current method, we achieve a 1.7 m...

    • maxnoe: Could you please give a reference for the ARTEMIS algorithm / results and why you think their...

    • burmist-git: Please have a look at this draft MR. It includes more explanations, and I hope it will clearly...

    • maxnoe: Aha! Whipple Artemis camera. I only found things related to ATLAS, which apparently also has/had...

    • maxnoe: You say std dev is currently about 1.7 m for LST, that's roughly equal to 2.3 * 0.75 m.

    • burmist-git: For completeness, I am attaching my slides as well. 083_LST_muons_08_ImpactPointReco.pdf

    • maxnoe: While you are looking into this, could you maybe also address #1279?

    • burmist-git: Yes i see. There are two main points: Observation plane: To avoid confusion between the tilted...

    • maxnoe: How do you compute the true value? I don't think we have code for that in ctapipe, but it would...

    • burmist-git: Actually, with this configuration, the simulated true core X and true core Y are the correct...

    • burmist-git: @maxnoe The initial implementation appears satisfactory. the next step is to estimate the...

    • burmist-git: This is without camera shadowing

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

  • Generalise table preprocessing #2791 [Voutsi] module:irf

    Closes #2790 Refactoring the DL2 processing for producing IRFs. The motivation is to use the same code for more...

  • Muon fit chord length fix hole radius correction m #2808 [burmist-git]

    Define correct hole radius of the reflector in for LST, MST and SST_1M in intensity_fitter.py Its issue : #2807

    • mexanick

      • mexanick: Suggested change help="Hole...

        • burmist-git: @mexanick We define the hole radius to correspond to the surface area of the hexagon. This is...

      • mexanick: Is this for SST-1M? Or for SST-2M (the CTAO-selected telescope)? If former, please update the...

        • burmist-git: @mexanick In the original code, the value was SST_1M, it appears to be same for SST.

        • mexanick: if it is the same, then just use "SST_" instead of "SST_1M_"

        • burmist-git: The last one is done

        • maxnoe: The SST is a dual mirror design, it needs completely different treatment. Please add the 1M back.

        • mexanick: This is a parameter, that's present in all telescopes. The way how it is treated is not...

        • maxnoe: But for the SST, you have the secondary mirror housing, the mirror hole and the camera to...

        • burmist-git: Sure, I'll change it back. That wasn’t the best change — better not to touch what was already there.

    • mexanick

    • maxnoe: Please do not remove the 1M, the whole radii for the different SSTs are not the same (and the 2M...

      • mexanick: hmm... @burmist-git claimed they are the same... If not, of course 1M should be added.

      • burmist-git: @maxnoe @mexanick For the SST, I could not find exact specifications, but from this drawing one...

    • maxnoe: This does not apply to all telescopes (e.g. MAGIC has square facets and the whole is two facets large).

      • maxnoe: I would just describe the configuration value here ( radius of the circle approximation of the...

      • burmist-git: Changed now

    • maxnoe

  • Muon fit chord length fix #2803 [burmist-git]

    Modified the chord_length function to fix a bug. It now filters out non-physical solutions for chodr vs phi. Updated...

    • kosack

    • maxnoe: I'm not sure why the sonar coverage is ill-computed... @kosack @maxnoe any idea? Could it be a...

    • maxnoe

    • maxnoe: Keep in mind this appears in the release change log: the context here is not clear. Add some...

    • maxnoe: Please remove this change. The function does not appear in the docs because it is not included...

      • maxnoe: THe file is also in the wrong directory.

      • mexanick: the function doesn't appear in the docs because it is decorated, not because it is not included...

      • maxnoe: How did you determine this? Sphinx only includes things in __all__, this is not in __all__ so it...

      • maxnoe: E.g. this function is also numba jitted and appears just fine in the...

      • mexanick: By trial-and-error, and then confirmed by web search.

      • burmist-git: @maxnoe I’ve removed it; please let me know.

    • maxnoe: See inline comments.

    • mexanick: I'm not sure why the sonar coverage is ill-computed... @kosack @maxnoe any idea? Could it be a...

    • maxnoe: Unfortunately this is a long-standing issue with slow progress: numba/numba#4268

    • mexanick: @kosack I just fixed the appearance of the chord_length function in the docs (it is a decorated...

    • mexanick

    • maxnoe: @burmist-git sorry, I discussed with @mexanick offline and we agreed that the docs should stay...

    • burmist-git: @maxnoe Sure, I can do that. The only remaining question is the directory of chord_length.rst....

    • maxnoe: The only remaining question is the directory There currently is no muon subdirectory in the...

    • mexanick

    • maxnoe: Ah, sorry, misread the commits.

    • maxnoe: Suggested change chord_length ...

    • maxnoe: You need to change the entry in the toc in docs/api-reference/image/muon.rst to take the changed...

    • 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 :

    • maxnoe

    • mexanick

  • Add averaging #2811 [burmist-git]

    Phi dist averaging

  • Move to new sonar server #2816 [maxnoe] no-changelog-needed

    These changes will only become effective once they are in main unfortunately... Here, CI will still use the old sonar server.

  • Fix sonar.yml for branch analysis #2818 [maxnoe]

  • Test new sonar config #2817 [maxnoe] no-changelog-needed

    Dummy PR with small docs change to test if new sonar config is working.

  • add impactpoint_intensity_fitter.py #2814 [burmist-git]

    An important error has been found and fixed (in the Chord calculation). It shows an improvement in the resolution...

    • maxnoe: If there is an issue in the intensity fitter, why add a second implementation instead of fixing it?

    • burmist-git: I fixed the problems and bugs I found so far. It does help, but the resolution is still not...

    • maxnoe: Note that the current intensity fit keeps the ring center fixed (opposed to just using it as an...

    • burmist-git: I still think adding more free parameters will make it worse. Moreover, it often fails to...

  • add MonitoringSource #2802 [TjarkMiener] API change Breaking change hdf5 data format

    This PR prepares ctapipe to interface with camera calibration data products from calibpipe via a new MonitoringSource().

    • TjarkMiener: Better to use here the read_table() function and select the first row (or single chunk), since...

    • mexanick

      • mexanick: Check if key is missing, and if so, raise an exception. Then you can simplify the nested...

      • mexanick: Expand the comment providing the reason why this is required (no proper time definition in...

      • mexanick: this should raise instead of warn

    • maxnoe: Why was the gain selection removed here?

      • TjarkMiener: it was moved some lines above

    • maxnoe: Instead of adding this to each event source, I would prefer a new, independent component...

    • maxnoe: Implementing the IO for monitoring data directly in SimTelEventSource is not a good fit. This...

      • TjarkMiener: Done

    • maxnoe: As commented inline, I think this must be implemented independently of any particular event...

    • TjarkMiener: Thanks for the valuable suggestion, @maxnoe! I wonder if this MonitoringSource can be a child...

    • maxnoe: A child? As in the MonitoringSource is an EventSource? No, that's exactly what I want to...

    • mexanick

    • TjarkMiener: @mexanick @maxnoe Can you please review the proposed MonitoringSource component (even though the...

    • maxnoe: Keeping with EventSource and the plugin system, I think MonitoringSource should be an abstract...

      • maxnoe: I think this should also be a TelescopeComponent that gets the subarray from the EventSource.

      • mexanick: I agree, what's implemented here is more a DL1MonitorinSource

      • maxnoe: This class should also get a plugin_entry_point so that plugins can provide...

      • TjarkMiener: Done and added to the pyproject

      • maxnoe: Please also add to ctapipe-info similar as --event-sources and --reconstructors

      • TjarkMiener: Done

    • mexanick

      • mexanick: We already have the MonitoringContainer, so I'm wondering if this shim is necessary. What would...

        • maxnoe: This definition is in line with the other ones we have. The Event has a container that collects...

        • TjarkMiener: Solved

      • mexanick: Suggested change def...

        • TjarkMiener: Solved

      • mexanick: Use a class member constant for at least /dl1/monitoring/telescope/ here and above, but also...

        • TjarkMiener: This will be addressed in future PRs as discussed in this PR.

      • mexanick: I don't think this is needed here.

        • TjarkMiener: Removed

      • mexanick: no, we document that we use "valid_from" convention. Checking that given monitoring file is...

        • TjarkMiener: True! Bounds are better treated in the current version anyway! Done

    • maxnoe: Having this as a free method will do a potentially expensive operation (converting time to mjd)...

      • mexanick: Also please update it to be get(timestamp: Any)->Any -> this is a getter, that can (and should)...

      • TjarkMiener: Done

    • mexanick: If you want to keep it generic (in a base class, to increase the code reuse), use...

      • TjarkMiener: Done

    • mexanick: you're importing a private method, which usually should not be needed. Instead, import...

      • TjarkMiener: Done

    • mexanick: use public interface ctapipe.io.metadata.Reference instead of a private method.

      • TjarkMiener: Done

    • mexanick: where self.has_pixel_statistics is initialized?

      • TjarkMiener: it's a class property

    • mexanick: Should be MonitoringSource class method (perhaps, private), get_monitoring_types, overloaded in...

      • TjarkMiener: Solved in current version

    • maxnoe: I'm not sure that we want to put the assumption of a single input file already into the base...

      • TjarkMiener: input_url moved to HDF5EventSource

    • maxnoe: Remove or move to the specific implementation, this assumes that the input file is HDF5.

      • TjarkMiener: Renamed to get_hdf5_monitoring_types()

    • maxnoe: Why is this part of the abstract interface? What about a MonitoringSource for pointing...

      • TjarkMiener: Now that you raise this point, I think we could have childs of the MonitoringSource based on the...

      • TjarkMiener: Nevermind, it will be a pain to initialize and apply in the process tool. We are also planning...

      • maxnoe: Not really a pain, I'd say, but yes a bit more complex to configure. It would be similar to the...

      • TjarkMiener: Process tool now supports multiple MonitoringSources

    • maxnoe: Also implement __enter__ and __exit__ here, so we have the guarantee that with can use the IO...

      • maxnoe: these can be abstractmethod or empty defaults.

      • TjarkMiener: Functions were added

    • TjarkMiener: Any reason to have the pointing separated from the monitoring, @maxnoe? Probably historically...

      • TjarkMiener: i.e. changing the containers to: # Pointing containers class...

    • maxnoe: You changed the path here to dl1

      • mexanick: this is intentional (and breaking change). In the future, we should have pointing information at...

      • maxnoe: The story is a bit more complicated. In older versions of ctapipe, we already filled simulation...

      • maxnoe: Also see #2655

      • mexanick: yeah, that's why I wasn't planning addressing pointing migration yet (not in this MR), but...

      • TjarkMiener: Reverted in the new version, please resolve the conversation if sufficient.

    • mexanick: I'd issue a warning here. If we reached this function, we expect to have something, aren't we?

      • TjarkMiener: A warning was added

    • TjarkMiener: @mexanick this is the part I meant earlier. If we do not provide a monitoring file it will try...

      • maxnoe: I think we should have a list of monitoring source names, which is empty by default class...

      • TjarkMiener: Good point !

    • mexanick

    • TjarkMiener: Okay, I'm happy with the PR! Thanks for the quick interaction @mexanick @maxnoe during dev...

    • maxnoe: What is it? Removed? Added? Or Modified?

      • TjarkMiener: okay I will polish this changelog more carefully given the fact that there are many breaking changes.

      • TjarkMiener: Done

    • maxnoe: Why is the number of entries used to compute the stats gone here? This is important information.

      • TjarkMiener: It's in the ChunkStatisticsContainer(StatisticsContainer), which is the Container returned by...

      • TjarkMiener: maybe we can move is_valid and n_events to the StatisticsContainer?

      • TjarkMiener: Done

    • mexanick: Suggested change * Remove containers...

    • mexanick: these keys are hardcoded in a number of places. As you're already on this, please factor them...

      • maxnoe: ctapipe.io.hdf5_data_format or similar

    • maxnoe: You could have just replaced key in most instances here. Now key is unused.

    • TjarkMiener

      • TjarkMiener: @TjarkMiener subarray here

      • TjarkMiener: Why we put the data level (top-level) here? Monitoring types should be decoupled from data levels.

        • maxnoe: Why? Monitoring information is related to data levels

      • TjarkMiener: I'm suggesting here: DRIVE_POSITIONS_GROUP = "/dl0/monitoring/telescope/pointing" And what is...

        • mexanick: I would leave it as is. In this group, you will have drive position, bending model corrections,...

        • mexanick: this is missed in all - perhaps causing the issue with the incomplete import during the doc build?

    • maxnoe: just curious... why does github syntax highlighting color FIXED_POINTING_GROUP differently? Any idea?

      • TjarkMiener: FIXED_POINTING_GROUP does not have an interger. Could this be?

      • TjarkMiener: Same for SIMULATION_*

    • maxnoe: I think it is fine to not expose these values at the ctapipe.io toplevel init. It's fine to have...

      • TjarkMiener: Just seeing it now! It should fix in the current version. Should be reverted?

      • maxnoe: I think this is very low-level info to be exposed at the top-level io module. So yes, I'd prefer...

      • TjarkMiener: Done

    • TjarkMiener: Should the DATA_MODEL_VERSION changed in the datawriter including the monitoring info?

    • maxnoe: mmh, something is going wrong with the coverage... Can you update with main?

    • maxnoe: docs build also fails with a circular import error: Extension error...

    • maxnoe: Use relative imports here, might fix the import error

      • maxnoe: if not, you can put the imports from ctapipe.io where needed, not at the top-level. I think the...

      • mexanick: no, this is properly circular import. PointingInterpolator is being used in io, but imports from...

      • maxnoe: I think it's ok in ctapipe.io.hdf5format, but either the import of the PointingInterpolator in...

      • mexanick: it is TableLoader that imports the PointingInterpolator, but that's not important. By...

      • maxnoe: HDF5EventSource also uses it: ctapipe/src/ctapipe/io/hdf5eventsource.py ...

      • mexanick: yes, I meant this particular issue was with TableLoader. I'm pretty sure there's more, and we...

      • maxnoe: Like I said, I think it makes semantical sense to keep the hdf5dataformat definitions in...

      • maxnoe: How can a data format be used beyond IO? I think it's rather a sign that the...

    • maxnoe: Since you have a close, you can call it here Suggested change ...

    • maxnoe: This looks mostly good to me, I'll make a detailed review pass next week. Since it's a pretty...

created at 2025-08-23 06:31 by dinghy 1.4.1.