Remove all hillas_paramters but version 5, fixes #866 #904
Conversation
maxnoe
changed the title
Codecov Report
@@ Coverage Diff @@
## master #904 +/- ##
========================================
- Coverage 82.7% 82.3% -0.4%
========================================
Files 186 186
Lines 10978 10709 -269
========================================
- Hits 9079 8814 -265
+ Misses 1899 1895 -4
Continue to review full report at Codecov.
|
|
codacy does not seam to support type annotations, can we change that? @kosack |
Yes, I've had this problem before - it seems to detect ctapipe as python 2.7... I couldn't find a way to change it - their help system was not useful (it just says it auto-detects it), and recommends an option to pylint that seem to have no effect. Maybe we can contact them. https://support.codacy.com/hc/en-us/articles/115002330985-Specifying-your-Python-version |
|
It could also be that their python 3 is < 3.5 which added type annotations. |
| @@ -690,7 +77,7 @@ def hillas_parameters_5(geom: CameraGeometry, image): | |||
| width, length = np.sqrt(eig_vals) | |||
|
|
|||
| # psi is the angle of the eigenvector to length to the x-axis | |||
| psi = np.arctan2(eig_vecs[1, 1], eig_vecs[0, 1]) | |||
| psi = np.arctan(eig_vecs[1, 1] / eig_vecs[0, 1]) | |||
There was a problem hiding this comment.
this is a change in implementation, I assume. Could you just shortly comment in the PR discussion (for later reference) why this change was needed and maybe which test was affected by this change (if at all).
There was a problem hiding this comment.
Ah I assume test_with_toy() (since it is a new test) discovered this little error and hence it was fixed within this PR?
There was a problem hiding this comment.
No, this was also discussed in Dortmund. It is basically the question what the support of the psi angle is. The size of the support was always π, but it was at different offsets.
Using np.arctan instead of arctan2 makes sure the interval is [-π/2, π/2]
There was a problem hiding this comment.
Not an error, just a very small improvement.
There was a problem hiding this comment.
Thanks for still writing it down here for reference
* master: (22 commits) Remove all hillas_paramters but version 5, fixes cta-observatory#866 (cta-observatory#904) Fix docstring of EventSeeker, fixes cta-observatory#768 (cta-observatory#914) Do not set autodownload, fixes doc build (cta-observatory#910) Import bokeh.palletes correctly, fixes cta-observatory#911 (cta-observatory#912) Fix SST-1M to be DC and not SC. Solves cta-observatory#905 (cta-observatory#908) Fix a few test warnings (cta-observatory#902) Updates of NectarCam and LSTCam real data eventsource class (cta-observatory#812) Implemented FACT image cleaning (cta-observatory#862) remove `config=None, tool=None` in some tests (cta-observatory#897) Remove flow submodule (got moved to its own package) (cta-observatory#893) Cleaning the ctapipe folder. this has been empty for 3 years. (cta-observatory#892) Additional metadata from pyhessio, similar to cta-observatory#655 (cta-observatory#895) add scikit-learn to install_requires (cta-observatory#877) Add .mailmap (cta-observatory#894) Fix subarray peek. No more warnings. (cta-observatory#841) SimTelEventSource using pyeventio (cta-observatory#864) Use sparse neighbor matrix (cta-observatory#826) Add test how it should be (cta-observatory#842) fix errordef > 0. (cta-observatory#839) Fix warning about already closed hessio file (cta-observatory#834) ... # Conflicts: # ctapipe/analysis/camera/chargeresolution.py # ctapipe/tools/extract_charge_resolution.py
No description provided.