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

Fix overwriting 4th coordinate in coord.jl #54

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Conversation

disberd
Copy link
Contributor

@disberd disberd commented Aug 18, 2021

I am studying a bit the internal of Proj4.jl to undertand how to exploit it better, but it seem that the current implementation on the new API wrongly overwrites the 4th coordinate if this is provided with a 4 element SVector as input.

I am studying a bit the internal of Proj4.jl to undertand how to exploit it better, but it seem that the current implementation on the new API wrongly overwrites the 4th coordinate if this is provided with a 4 element SVector as input.
@visr
Copy link
Member

visr commented Aug 18, 2021

Thanks! Indeed this looks like an unfortunate copy paste error.

Could you perhaps add a test for this? The last one from this block shows the effect of passing a non-Inf time coordinate:
https://github.com/JuliaGeo/Proj4.jl/blob/b00cad114be5c90a91072eb1a0d96f5b4717d4d0/test/applications.jl#L79-L88

Since you need network for that test, perhaps it is best to add the test here:
https://github.com/JuliaGeo/Proj4.jl/blob/b00cad114be5c90a91072eb1a0d96f5b4717d4d0/test/proj6api.jl#L281

And feel free to already bump the patch version, such that we can tag a release right after.

@disberd
Copy link
Contributor Author

disberd commented Aug 18, 2021

Thanks! Indeed this looks like an unfortunate copy paste error.

Could you perhaps add a test for this? The last one from this block shows the effect of passing a non-Inf time coordinate:
https://github.com/JuliaGeo/Proj4.jl/blob/b00cad114be5c90a91072eb1a0d96f5b4717d4d0/test/applications.jl#L79-L88

Since you need network for that test, perhaps it is best to add the test here:
https://github.com/JuliaGeo/Proj4.jl/blob/b00cad114be5c90a91072eb1a0d96f5b4717d4d0/test/proj6api.jl#L281

And feel free to already bump the patch version, such that we can tag a release right after.

Hi, Thanks for the fast reply.

I tried looking at the test you suggested but I do not understand why you say that one would need the network to perform this kind of test.
Looking at the file, I decided to put this test in the single transformation test-set rather than the network and it seems that the tests correctly fail in master but passes in this patched version.
I never did a test before so sorry if I wrongly assumed I did not need network and put the test in the wrong place.

@visr
Copy link
Member

visr commented Aug 18, 2021

I tried looking at the test you suggested but I do not understand why you say that one would need the network to perform this kind of test.

You're right, the test you added also properly tests if the time coordinate is not overwritten. I was thinking about the other test with networking because that is an example of a transformation where the time actually affects the x and y coordinate transformations, to show that it is not only passed along, but also does something. But that isn't really needed to test this fix, your solution is simpler.

@visr visr merged commit 9a63d75 into JuliaGeo:master Aug 18, 2021
@disberd disberd deleted the patch-1 branch August 18, 2021 14:59
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