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 CRS constructor. #74

Merged
merged 9 commits into from
Dec 22, 2022
Merged

Add CRS constructor. #74

merged 9 commits into from
Dec 22, 2022

Conversation

evetion
Copy link
Member

@evetion evetion commented Nov 4, 2022

No description provided.

@evetion evetion requested a review from visr November 4, 2022 11:06
@evetion evetion marked this pull request as ready for review November 4, 2022 17:34
Copy link
Member

@visr visr 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. I added some comments.

src/crs.jl Show resolved Hide resolved
test/libproj.jl Show resolved Hide resolved
src/crs.jl Show resolved Hide resolved
src/crs.jl Outdated
Comment on lines 54 to 83
function is_geographic(crs::CRS)
proj_get_type(crs.pj) in (
PJ_TYPE_GEOGRAPHIC_CRS,
PJ_TYPE_GEOGRAPHIC_2D_CRS,
PJ_TYPE_GEOGRAPHIC_3D_CRS)
end

function is_projected(crs::CRS)
proj_get_type(crs.pj) == PJ_TYPE_PROJECTED_CRS
end
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this covers everything? What about PJ_TYPE_TEMPORAL_CRS or PJ_TYPE_COMPOUND_CRS? Could they also be geographic or projected? If we can't be certain, perhaps we should just stick to the PROJ API and add proj_get_type(::CRS) for slightly more convenience.

@evetion
Copy link
Member Author

evetion commented Dec 15, 2022

@rafaqz This PR includes a method to convert between GFT instances, like ArchGDAL.

@evetion
Copy link
Member Author

evetion commented Dec 18, 2022

Not sure why the tests are suddenly failing, like the PROJ version changed in between, but can't replicate it. @visr, any thoughts?

@visr
Copy link
Member

visr commented Dec 19, 2022

That's your own show change from #73 ;)

Fixed in 29e3415

The new show is better, it shows explicitly it is a no-op (old then new):

Transformation
    source: WGS 84
    target: WGS 84
    direction: forward

Transformation noop
    description: Null geographic offset from WGS 84 to WGS 84
    definition: proj=noop ellps=GRS80
    direction: forward

@evetion
Copy link
Member Author

evetion commented Dec 19, 2022

LGTM

@visr
Copy link
Member

visr commented Dec 19, 2022

It would be better to directly do this in the style of yeesian/ArchGDAL.jl#349 and JuliaGeo/LibGEOS.jl#149, since this adds a bunch of methods like proj_get_id_code(crs::CRS) = proj_get_id_code(crs.pj) that would soon be removed again.

@evetion
Copy link
Member Author

evetion commented Dec 19, 2022

The style being
Base.unsafe_convert(::Type{Ptr{Cvoid}}, x::CRS) = x.ptr
?

@visr
Copy link
Member

visr commented Dec 19, 2022

Yes indeed, then we don't need to take out the pointer ourselves, which is less code and safer for GC.

@evetion evetion requested a review from visr December 21, 2022 06:58
@visr visr merged commit 91db74b into master Dec 22, 2022
@visr visr deleted the feat/crs branch December 22, 2022 21:03
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.

2 participants