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

Remove unused and untested muon fitting code #1254

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Apr 6, 2020

First step in refactoring of the muon code.

This removed a bunch of unused and untested functions, added by myself four years ago.

Maybe I'll add some of these back, but then by integrating it with the other code and with tests added.

So for now, let's just purge this.

kosack
kosack previously approved these changes Apr 6, 2020
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.

If it's not being used elsewhere, I'm ok with it.

@maxnoe
Copy link
Member Author

maxnoe commented Apr 6, 2020

At least not in the LST muon code

@maxnoe
Copy link
Member Author

maxnoe commented Apr 6, 2020

Which I'm going to refactor as well along with this...

return result


def efficiency_fit(
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this efficiency_fit the one used to calculate the muon_efficiency given as the output of the muon code?

Copy link
Member Author

@maxnoe maxnoe Apr 7, 2020

Choose a reason for hiding this comment

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

No, this is a different, untested function doing the same thing.

rlopezcoto
rlopezcoto previously approved these changes Apr 7, 2020
vuillaut
vuillaut previously approved these changes Apr 7, 2020
@kosack
Copy link
Contributor

kosack commented Apr 7, 2020

looks like you need to remove vacanti94 from bibliography.rst. It's somewhat annoying that you need to use all references, or they cause an error, but I don't see a way to disable that.

/home/travis/build/cta-observatory/ctapipe/docs/bibliography.rst:13:Citation [vacanti94] is not referenced.
Error: Error building sphinx doc

Or alternately, mention this reference somewhere.

@maxnoe maxnoe dismissed stale reviews from vuillaut, rlopezcoto, and kosack via 893b985 April 7, 2020 12:22
kosack
kosack previously approved these changes Apr 7, 2020
@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #1254 into master will increase coverage by 0.51%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
+ Coverage   86.78%   87.30%   +0.51%     
==========================================
  Files         192      192              
  Lines       11967    12129     +162     
==========================================
+ Hits        10386    10589     +203     
+ Misses       1581     1540      -41     
Impacted Files Coverage Δ
ctapipe/image/muon/fitting.py 100.00% <ø> (+45.36%) ⬆️
ctapipe/conftest.py 100.00% <0.00%> (ø)
ctapipe/io/sources.py 0.00% <0.00%> (ø)
ctapipe/image/reducer.py 100.00% <0.00%> (ø)
ctapipe/image/toymodel.py 100.00% <0.00%> (ø)
ctapipe/coordinates/__init__.py 100.00% <0.00%> (ø)
ctapipe/core/tests/test_traits.py 100.00% <0.00%> (ø)
ctapipe/coordinates/ground_frames.py 100.00% <0.00%> (ø)
ctapipe/image/tests/test_extractor.py 100.00% <0.00%> (ø)
ctapipe/coordinates/telescope_frame.py 100.00% <0.00%> (ø)
... and 48 more

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 0db9655...143cf10. Read the comment docs.

@maxnoe maxnoe added this to the v0.8.0 milestone Apr 7, 2020
@maxnoe
Copy link
Member Author

maxnoe commented Apr 8, 2020

@kosack could you re-approve

@maxnoe maxnoe merged commit a2769b3 into master Apr 14, 2020
@maxnoe maxnoe deleted the remove_unused_muon_functions branch April 14, 2020 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants