search

panoptes / panoptes-utils

A set of Astronomical utility functions. PANOPTES style. :hammer_and_wrench: :telescope: :stars:
https://panoptes-utils.readthedocs.io
MIT License
4 stars 9 forks source link

revamp horizon class #281

Closed danjampro closed 3 years ago

danjampro commented 3 years ago
codecov[bot] commented 3 years ago

Codecov Report

Merging #281 (6e20526) into develop (ff51019) will increase coverage by 0.19%. The diff coverage is 96.61%.

:exclamation: Current head 6e20526 differs from pull request most recent head 8f64a69. Consider uploading reports for the commit 8f64a69 to get more accurate results Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #281      +/-   ##
===========================================
+ Coverage    74.66%   74.85%   +0.19%     
===========================================
  Files           31       31              
  Lines         2088     2112      +24     
===========================================
+ Hits          1559     1581      +22     
- Misses         529      531       +2
Impacted Files Coverage Δ
src/panoptes/utils/horizon.py 96.77% <96.61%> (-3.23%) :arrow_down:

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 ff51019...8f64a69. Read the comment docs.

danjampro commented 3 years ago

@wtgee I don't think your interpretation of horizon_line is correct. I think it is an array of altitudes at each integer azimuth as per the current code:

https://github.com/panoptes/panoptes-utils/blob/ff51019cdd0e188cf5e8d8d70fc3579776a31716/src/panoptes/utils/horizon.py#L73

In your comment you say: "The horizon_line here switched between being an array of altitudes instead of an array of azimuths."

But then go on to write: self.horizontal_line = self.alt_line

Which as far as I can tell is consistent with the existing changes, with the exception of the mistyped horizontal_line, which should be horizon_line.

EDIT: There is a mistake in the suggested changes after all. The correct version looks like this:

        self.horizon_line = np.zeros(360, dtype="float")
        for i in range(360):
            self.horizon_line[i] = self.get_horizon(i).to_value(u.deg)
danjampro commented 3 years ago

Not sure how the old code supports negative horizons: https://github.com/panoptes/panoptes-utils/blob/ff51019cdd0e188cf5e8d8d70fc3579776a31716/src/panoptes/utils/horizon.py#L61