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

add simple projected coordinates #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Sep 1, 2022

almost a copy of the implementation in SkyImages.jl

would be useful to upstream to SkyCoords, if this is in scope of the package

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Base: 91.52% // Head: 94.61% // Increases project coverage by +3.08% 🎉

Coverage data is based on head (56fbeae) compared to base (6c8849b).
Patch coverage: 93.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   91.52%   94.61%   +3.08%     
==========================================
  Files           3        4       +1     
  Lines         118      130      +12     
==========================================
+ Hits          108      123      +15     
+ Misses         10        7       -3     
Impacted Files Coverage Δ
src/SkyCoords.jl 98.66% <ø> (+5.33%) ⬆️
src/projected.jl 91.66% <91.66%> (ø)
src/types.jl 88.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@mileslucas mileslucas left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @aplavin! I get a little wary of code like this that starts doing lots of Type gymnastics- it's really easy to miss a corner case when constructors and converts get written this way. I would prefer to see an exhaustive test suite that uses a for loop to check all types work with projected coords (for both projected types).

I'm curious about your thoughts of defining Base.parent and using that in addition to or instead of SkyCoords.origin? Also, should SkyCoords.separation and SkyCoords.position_angle be defined for the projected offsets (i.e., we can measure the direct distance of the offset from the origin, but do we want that?)?

src/projected.jl Outdated Show resolved Hide resolved
src/projected.jl Outdated Show resolved Hide resolved
src/projected.jl Outdated Show resolved Hide resolved
@aplavin
Copy link
Contributor Author

aplavin commented Sep 13, 2022

I'm curious about your thoughts of defining Base.parent and using that in addition to or instead of SkyCoords.origin?

I'm neutral on that. Typically, Base.parent is used for arrays, but can be added here as well.
I definitely prefer to keep origin either way, because it's the more intuitive name in coordinates context.

Maybe, parent_coords_type should be renamed to origin_type, if kept at all? Then, I think, overloading parent would make less sense.

Also, should SkyCoords.separation and SkyCoords.position_angle be defined for the projected offsets

Now, offsets are just a SVector{2}s. Don't think we should overload these funcs for them...

@mileslucas
Copy link
Member

I definitely prefer to keep origin either way, because it's the more intuitive name in coordinates context.

Sounds good

Also, should SkyCoords.separation and SkyCoords.position_angle be defined for the projected offsets

Now, offsets are just a SVector{2}s. Don't think we should overload these funcs for them...

I mean like so:

using LinearAlgebra
separation(proj::ProjectedCoords) = norm(proj.offset)

@aplavin aplavin force-pushed the proj branch 2 times, most recently from b8dcfcf to 8a6ab33 Compare November 10, 2022 13:48
@aplavin
Copy link
Contributor Author

aplavin commented Dec 23, 2022

Bump...
Anything else remains to be addressed in the code?

@mileslucas mileslucas self-requested a review December 27, 2022 10:04
src/types.jl Outdated Show resolved Hide resolved
lon(c::AbstractProjectedCoords) = lon(origin(c)) + c.offset[1] / cos(lat(origin(c)))
lat(c::AbstractProjectedCoords) = lat(origin(c)) + c.offset[2]

Base.convert(::Type{T}, c::T) where {T<:AbstractProjectedCoords} = c
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Base method is less specific and won't be called when converting skycoords. Same motivation as

Base.convert(::Type{T}, c::T) where {T<:AbstractSkyCoords} = c
.

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