Skip to content
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

Restore apply_time_delta_cleaning as a public method #1106

Merged
merged 3 commits into from
Jul 16, 2019

Conversation

watsonjj
Copy link
Contributor

apply_time_delta_cleaning is a useful function that we started utilising in the CHEC cleaning. I suggest it is restored as a public method.

maxnoe
maxnoe previously approved these changes Jul 15, 2019
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@maxnoe
Copy link
Member

maxnoe commented Jul 15, 2019

Are there any tests for this function?

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #1106 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   84.48%   84.51%   +0.03%     
==========================================
  Files         182      182              
  Lines       11220    11242      +22     
==========================================
+ Hits         9479     9501      +22     
  Misses       1741     1741
Impacted Files Coverage Δ
ctapipe/image/tests/test_cleaning.py 100% <100%> (ø) ⬆️
ctapipe/image/cleaning.py 96.29% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62df71b...222ef66. Read the comment docs.

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a unit test for apply_time_delta_cleaning. It probably got covered in the test for FACT cleaning, but if we use it on its own outside of that function, it should have a dedicated unit test. Otherwise no problem to make this public again.

ctapipe/image/cleaning.py Outdated Show resolved Hide resolved
@watsonjj watsonjj requested review from kosack and maxnoe July 16, 2019 07:57
@kosack kosack merged commit b831f8c into cta-observatory:master Jul 16, 2019
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Jul 16, 2019
* upstream/master:
  Restore apply_time_delta_cleaning as a public method (cta-observatory#1106)
  Update mailmap (cta-observatory#1103)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants