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

Fix set_line_hillas #974

Merged
merged 4 commits into from
Feb 20, 2019
Merged

Conversation

kpfrang
Copy link
Member

@kpfrang kpfrang commented Feb 20, 2019

The current implementation of set_line_hillas did not work for huge values of m. This is an quick fix which calculates the actual start end end points of the line exactly. This works also in the case of (almost) vertical lines as you can see in the plot attached.

event0

thomasgas
thomasgas previously approved these changes Feb 20, 2019
ctapipe/visualization/mpl_array.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #974 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #974      +/-   ##
==========================================
- Coverage   77.74%   77.74%   -0.01%     
==========================================
  Files         191      191              
  Lines       10831    10829       -2     
==========================================
- Hits         8421     8419       -2     
  Misses       2410     2410
Impacted Files Coverage Δ
ctapipe/visualization/mpl_array.py 96.07% <100%> (-0.08%) ⬇️

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 79ad0e0...6506f2c. Read the comment docs.

@maxnoe
Copy link
Member

maxnoe commented Feb 20, 2019

While you are at it, can you replace the .value with .to_value(u.m), this makes sure it's always meters and not just uses the internally stored unit.

@kpfrang kpfrang force-pushed the fix_set_line_hillas branch from 2a39fb8 to 996bbeb Compare February 20, 2019 12:17
maxnoe
maxnoe previously approved these changes Feb 20, 2019
@maxnoe
Copy link
Member

maxnoe commented Feb 20, 2019

Nice, let's just wait for travis

@kpfrang
Copy link
Member Author

kpfrang commented Feb 20, 2019

There was some accidental typo, which is fixed now.

@maxnoe maxnoe merged commit 0db9f6d into cta-observatory:master Feb 20, 2019
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.

3 participants