Fix issue#3 #97
Fix issue#3 #97
Conversation
src/gcirc.jl
Outdated
| # This file is a part of AstroLib.jl. License is MIT "Expat". | ||
| # Copyright (C) 2016 Mosè Giordano. |
There was a problem hiding this comment.
As a general remark, removing copyright notice isn't a nice thing to do 🙂
src/gcirc.jl
Outdated
| @@ -42,66 +23,124 @@ end | |||
|
|
|||
| ### Purpose ### | |||
|
|
|||
| Computes rigorous great circle arc distances. | |||
| Computes rigorous great circle arc distances using modern Julia astronomical coordinates. | |||
There was a problem hiding this comment.
This comment feels unnecessary.
src/gcirc.jl
Outdated
| * The function `sphdist` provides an alternate method of computing a spherical | ||
| distance. | ||
| * The Haversine formula can give rounding errors for antipodal points. | ||
|
|
||
| Code of this function is based on IDL Astronomy User's Library. |
There was a problem hiding this comment.
What's the motivation for deleting this?
src/gcirc.jl
Outdated
| if units == :radians | ||
| new{T}(SkyCoords.ICRSCoords(ra, dec)) | ||
| elseif units == :hours_degrees | ||
| new{T}(SkyCoords.ICRSCoords(ra * π/12.0, deg2rad(dec))) | ||
| elseif units == :degrees | ||
| new{T}(SkyCoords.ICRSCoords(deg2rad(ra), deg2rad(dec))) | ||
| else | ||
| throw(ArgumentError("units must be :radians, :hours_degrees, or :degrees")) | ||
| end |
There was a problem hiding this comment.
Should this use https://github.com/JuliaAstro/UnitfulAstro.jl?
src/gcirc.jl
Outdated
| struct AstroCoordinate{T<:AbstractFloat} | ||
| coord::SkyCoords.ICRSCoords | ||
|
|
||
| function AstroCoordinate(ra::T, dec::T, units::Symbol=:radians) where T<:AbstractFloat |
There was a problem hiding this comment.
Why is this an inner constructor?
src/gcirc.jl
Outdated
| units_sym = if units == 0 | ||
| :radians | ||
| elseif units == 1 | ||
| :hours_degrees | ||
| elseif units == 2 | ||
| :degrees | ||
| else | ||
| throw(DomainError(units, "units must be 0 (radians), 1 (hours, degrees) or 2 (degrees)")) | ||
| end |
There was a problem hiding this comment.
Also here, seems to be something to integrate with UnitfulAstro.
src/gcirc.jl
Outdated
| gcirc(units::Integer, radec1::Tuple{Real, Real}, radec2::Tuple{Real, Real}) = | ||
| gcirc(units, radec1..., radec2...) | ||
| # Helper function for converting radians to arcseconds | ||
| rad2sec(x::T) where T<:AbstractFloat = x * T(206264.8062470963) |
|
I am getting a lot of trouble in the compt test failures, though I am using the suggested compt preview results
|
thanks for urvaluable review will update my pr Discription |
|
Looks like your tests are failing because you are trying to use AstroAngles.jl which has a minimum Julia version of 1.3 while AstroLib.jl is being tested against 1.0. I don't use this package much but from a design perspective I'm not a big fan of this change. I think there is utility in providing a common (or at least similar) interface to IDL in this application to ease the transition for new users and to enable easier translation of legacy IDL code. In the case that someone wants a more Julian interface I think we should be encouraging them to switch over to using SkyCoords.jl directly rather than offering some sort of middle ground that we have to maintain separately from SkyCoords.jl itself. |
|
Yeah it can be better option |
#3
SkyCoords.jlingcirc.jlto make it more compatible-notebook I have tested using some examples, u can prefer my notebook