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

Bug: fix geographic coordinate conversions #4839

Merged

Conversation

chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Feb 29, 2024

The latitude to theta conversion (EDIT: as well as the longitude to phi and to convert_to_cartesian method) in the geographic coordinate handlers was incorrect in a number of locations. I added a new test and parametrized the existing geographic coordinate tests while I was there.

matthewturk
matthewturk previously approved these changes Feb 29, 2024
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

Can you verify this matches what's in the docs?

@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Feb 29, 2024
@chrishavlin
Copy link
Contributor Author

Just took a look through the docs and I can't find any mention of latitude/theta conversion. There are some scattered comments related to the aitoff projection that mention e.g., "latitude is -PI/2 to PI/2 (latitude = PI/2 - colatitude)", but I don't see any user-facing docs that do.

Checking on that prompted me to look at longitude as well and I realized the _longitude_to_phi function has an error as well... going to push up a fix in a moment.

@chrishavlin
Copy link
Contributor Author

@yt-fido test this please

@chrishavlin chrishavlin changed the title [Bug] fix latitude-theta conversion Bug: fix geographic coordinate conversions Feb 29, 2024
@chrishavlin
Copy link
Contributor Author

@yt-fido test this please

1 similar comment
@neutrinoceros
Copy link
Member

@yt-fido test this please

@chrishavlin
Copy link
Contributor Author

pre-commit.ci autofix

@chrishavlin
Copy link
Contributor Author

Assuming tests still pass, this should be good to go. @matthewturk any more comments here?

@chrishavlin
Copy link
Contributor Author

@yt-fido test this please

1 similar comment
@chrishavlin
Copy link
Contributor Author

@yt-fido test this please

@neutrinoceros
Copy link
Member

@matthewturk are you able to take another look ? I may be able to fill in as a reviewer otherwise.

@chrishavlin
Copy link
Contributor Author

@neutrinoceros I checked in w Matt, he'll be able to take another look soon

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

This is great.

@neutrinoceros neutrinoceros merged commit 24736e1 into yt-project:main Apr 3, 2024
13 checks passed
@neutrinoceros
Copy link
Member

thanks all !

Copy link

lumberbot-app bot commented Apr 3, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout yt-4.3.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 24736e144bd573bf170fd183944caf6f8129f2cc
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #4839: Bug: fix geographic coordinate conversions'
  1. Push to a named branch:
git push YOURFORK yt-4.3.x:auto-backport-of-pr-4839-on-yt-4.3.x
  1. Create a PR against branch yt-4.3.x, I would have named this PR:

"Backport PR #4839 on branch yt-4.3.x (Bug: fix geographic coordinate conversions)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants