-
Notifications
You must be signed in to change notification settings - Fork 269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change ImageCleaner
API
#2511
Change ImageCleaner
API
#2511
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2511 +/- ##
=======================================
Coverage 92.53% 92.53%
=======================================
Files 235 235
Lines 20063 20064 +1
=======================================
+ Hits 18565 18566 +1
Misses 1498 1498 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why TailCutsDataVolumeReducer
was changed as well here? This change seems to remove the ability to change which image cleaner implementation is used inside it, but that is not mentioned at all in the changelog, and seems to be outside of the scope of the PR (aside from the fact that that DataVolumeReducer is confusingly named and should be ImageCleaningDataVolumeReducer
or similar, as it is not tail-cuts specific)
Why would something called |
I agree and mentioned that in the request, but that reducer was configurable, presumably for a reason. I suspect more that it was just misnamed, rather than that it should be restricted to one type of cleaning. All it does is iteratively clean and dilate, which should work for any cleaning implementation. Nevertheless, that should be a different PR if you want to change how it works, and the ACADA people should be involved. |
Ok... I think we discussed this already elsewhere: keep the old signature in a method like: Then the |
Thanks for the comments! The TwoStepExtractor is already using the low level I'll create another PR for renaming the |
Maybe a better change would be to for now just add the event as new optional parameter? def __call__(self, tel_id, image, peak_time=None, *, event=None): That is a) backwards compatible |
- Switching from low-level cleaning method approach to just adding event as keyword argument to the __call__ function - backwards compatible now
cd4d02b
to
4317c30
Compare
Just to summarize from the discussions that are now in collapsed threads: we decided to not pass the full event, but just the |
Change API of
ImageCleaner
to be__call__(self, tel_id: int, event: ArrayEventContainer)
. Closes #2015.@aleberti