Add another cleaning method in cleaning.py #1672
Conversation
This reverts commit de9b878.
Codecov Report
@@ Coverage Diff @@
## master #1672 +/- ##
==========================================
+ Coverage 91.97% 92.04% +0.07%
==========================================
Files 189 189
Lines 14910 14877 -33
==========================================
- Hits 13713 13694 -19
+ Misses 1197 1183 -14
Continue to review full report at Codecov.
|
…aning() & a small code modification
|
@satoshifukami0115 The brightest_island function is now merged and you can update here to use it. |
|
@maxnoe Thank you! I changed the part to use this function. |
|
This looks good, only a test is missing @satoshifukami0115 There is also be a corresponding |
|
Thank you @maxnoe. The test means to include a test function in this file, right? I will also work on the Cleaner class within this week and let you know once done. |
|
@satoshifukami0115 Yes, exactly. Add a new test function there that tests if this cleaning works as intended, preferably by including some hand-crafted dummy images where you know which pixels should survive or not. |
There was a problem hiding this comment.
looks good. Could you please also run black to re-format the code? Actually the correct method should be to install the git commit hook by first running this as shown in the Getting Started for Developers guide for ctapipe:
pre-commit install
After that, it should be automatically formatted when you commit. (you may have some commit failures if the code isn't already formatted, so just add/commit twice in that case)
ctapipe/image/cleaning.py
Outdated
| Apply MAGIC-like image cleaning with timing information. See `ImageCleaner.__call__()` | ||
| """ | ||
|
|
||
| return mars_cleaning_1st_pass( |
There was a problem hiding this comment.
Do you really want to run this function here, and not your time_constrained_clean() function?
There was a problem hiding this comment.
No, this I should have changed to time_constrained_clean(). I will do it now...
ctapipe/image/cleaning.py
Outdated
| `image[~mask] = 0` | ||
|
|
||
| """ | ||
| pixels_to_remove = [] |
There was a problem hiding this comment.
Yes, it is. I will remove this line...
ctapipe/image/cleaning.py
Outdated
| mask_boundary = ( pixels_above_boundary & pixels_with_picture_neighbors ) & np.invert(mask_core) | ||
|
|
||
| # keep boundary pixels whose arrival times are within a certain time limit of the neighboring core pixels | ||
| pixels_to_remove = [] |
There was a problem hiding this comment.
Same here: this is an unused variable?
There was a problem hiding this comment.
Yes, same here.
@kosack Thank you for your review. I did not run pre-commit. I will do it and push the branch again. |
|
The pyflakes check is also complaining about some unused variables: |
…_clean() inside TimeConstrainedImageCleaner
Yes, it seems that the |
|
I have performed the modifications you suggested and also confirmed that |
|
@satoshifukami0115 Sorry for the late reply, this looks good know. Feel free to ping me the next time something is ready for review and I am not reacting for so long. |
I tried to implement a new image cleaning method used in MAGIC.
Distributions of a few Hillas parameters are checked in the link below.
https://indico.cta-observatory.org/event/3368/contributions/28400/attachments/19171/26360/CTA_ASWG_20210331_Fukami.pdf
I'm not sure if this method is better than other methods so far, but I think we can have an additional method as one option.
If you have any suggestions or comments, please let me know.