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 coords convert_to_cartesian with units #4893

Merged

Conversation

chrishavlin
Copy link
Contributor

Close #4892

@chrishavlin chrishavlin changed the title BUG: geographic coords conver_to_cartesian with units BUG: Fix geographic coords convert_to_cartesian with units May 2, 2024
@chrishavlin
Copy link
Contributor Author

just confirming, the code snippet from the issue now runs as expected

import numpy as np
dshp = (16,16,16)
data = {'density': np.random.random(dshp)}

bbox = np.array([[0, 1000], 
                [-150., -100.], 
                [-90., 90.]])
ds = yt.load_uniform_grid(data, dshp, geometry='geographic', 
                          bbox = bbox,
                          axis_order=('altitude', 'longitude', 'latitude'))

yt.SlicePlot(ds, 'altitude', 'density', window_size=(3,2))

@chrishavlin
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I'd prefer if we used colatitude instead of theta internally, and avoid shorthands while we're at it: _lat_to_theta -> _latitude_to_colatitude

Other than that, LGTM

@chrishavlin
Copy link
Contributor Author

chrishavlin commented May 3, 2024

I'd prefer if we used colatitude instead of theta internally,

good idea. would save me from adding comments to remind myself later that theta is colatitude :)

I'll make those changes.

@neutrinoceros neutrinoceros merged commit 057f9ae into yt-project:main May 6, 2024
13 checks passed
@neutrinoceros neutrinoceros added this to the 4.4.0 milestone May 6, 2024
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.

BUG: convert_to_cartesian from _sanitize_center fails for Geographic geometry when bbox is subset of globe
2 participants