Add iterative tailcuts cleaning variant #2329
Conversation
clara-escanuela
changed the title
|
I'd suggest making this a separate cleaning function, rather than modifying TailCuts (which has already been modified so many times that it hardly looks simple anymore). Since tailcuts is the default algorithm, it's best to keep it simple. Another possibility is to make this algorithm call the tailcuts function, rather than modify it. That might require a small modification, but better than adding a loop to tailcuts_clean where there wasn't one before. You will also need to add a new ImageCleaner subclass. |
|
Maybe also have a look at this part of the VolumeReducer, i think its quite similar. Maybe you can write a function used in both classes? |
That would make sense I think. Or can we somehow alter the "dilate" function so that it has the option to only dilate pixels above certain threshold? |
|
I think it is better to just add it as an extra cleaning method, otherwise dilate would not be so general and more input parameters would be required |
|
By the way, what is the reason for CI / docs to fail? The error message is not so clear to me and I am curious as this is also happening in my other PR |
clara-escanuela
force-pushed
the
tailcuts_hysteresis
branch
from
0644a62
to
6e67416
Compare
maxnoe
changed the title
| # AND have any neighbor that is in the picture | ||
| pixels_above_boundary = image >= boundary_thresh | ||
|
|
||
| for count in range(max_iter): |
There was a problem hiding this comment.
I know we both complained that this is a new cleaning method, not just tailcuts,but in the end the code is nearly 99% copied from tailcuts. So maybe:
- we reconsider and just add an iteration option to tailcuts_clean (even if it's a bit ugly and probably breaks processing all events at once)
- factor out the common code in both functions to a separate function that can be called by
tailcuts_cleanandtailcuts_hysteresis_clean - keep as is, but have lots of code duplication and the very real posisbility of forgetting to fix bugs in multiple places.
@maxnoe what do you think?
ctapipe/image/cleaning.py
Outdated
| pixels_above_boundary & pixels_with_picture_neighbors | ||
| ) | pixels_in_picture | ||
| else: | ||
| pixels_with_boundary_neighbors = geom.neighbor_matrix_sparse.dot( |
There was a problem hiding this comment.
You can move this out of the loop because it is a constant
There was a problem hiding this comment.
yes this will gain you a bit of speed as well, since pixels_above_boundary is not changing, there is no need to do the dot-product each time
clara-escanuela
force-pushed
the
tailcuts_hysteresis
branch
from
6ed78dc
to
0fa134b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2329 +/- ##
==========================================
+ Coverage 92.47% 92.49% +0.02%
==========================================
Files 234 234
Lines 19965 20020 +55
==========================================
+ Hits 18462 18517 +55
Misses 1503 1503 ☔ View full report in Codecov by Sentry. |
This would be my approach for Issue #2326