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: migrate away from broken optional dependency (pyregion -> regions) #4235

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

neutrinoceros
Copy link
Member

PR Summary

Should fix #4234

@neutrinoceros neutrinoceros added this to the 4.1.3 milestone Dec 5, 2022
matthewturk
matthewturk previously approved these changes Dec 5, 2022
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Dec 5, 2022

two notes:

Turns out regions isn't a drop-in replacement for pyregion so we still need to adapt the code somehow

AttributeError: module 'regions' has no attribute 'parse'

Also, at least one of the notebooks that uses it in the docs is currently broken for a different reason:
https://yt-project.org/docs/dev/cookbook/fits_radio_cubes.html?highlight=fits%20radio

...
ValueError: Invalid vmin or vmax

This other problem is not my target here so if I can't find a solution I'll report it separately.
For information ping @jzuhone, who I assume is more knowledgeable than me on this part of the code

@neutrinoceros neutrinoceros removed this from the 4.1.3 milestone Dec 5, 2022
@neutrinoceros
Copy link
Member Author

I may have opened this much too soon. Migration guidelines do not even exist yet, but here's where to follow progress on it, eventually: astropy/regions#488

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Dec 6, 2022

With a little help from regions people (astropy/regions#490) I was able to complete the migration;
This can be checked with this snippet (slightly adapted from our docs):

import yt
from yt.frontends.fits.misc import ds9_region

ds = yt.load("grs-50-cube.fits.gz")
region = 'galactic;box(+49:26:35.150,-0:30:04.410,1926.1927",1483.3701",0.0)'
box_reg = ds9_region(ds, region)

print(box_reg.quantities.extrema(("fits", "temperature")))

prj = yt.ProjectionPlot(
    ds,
    "z",
    ("fits", "temperature"),
    origin="native",
    data_source=box_reg,
    weight_field=("index", "ones"),
)  # "ones" weights each cell by 1
prj.set_zlim(("fits", "temperature"), 1.0e-2, 1.5)
prj.set_log(("fits", "temperature"), True)
prj.save("/tmp/")

which yields
grs-50-cube fits gz_Projection_z_temperature_ones

(maybe the plot shouldn't have so much empty space in it but that's a different story, I think)

I will open this PR to review when I can verify that the docs build correctly

@neutrinoceros neutrinoceros marked this pull request as ready for review December 6, 2022 12:29
@neutrinoceros
Copy link
Member Author

Not sure if this should be backported to the 4.1.x branch since regions isn't API stable yet, but I'll leave that decision up for discussion.

@matthewturk
Copy link
Member

@neutrinoceros I think unless we get a bug report, we shouldn't at this point.

@neutrinoceros
Copy link
Member Author

fine by me !

@neutrinoceros
Copy link
Member Author

@matthewturk do you want to push the button ?

@matthewturk matthewturk merged commit 8a33df0 into yt-project:main Dec 7, 2022
@neutrinoceros neutrinoceros deleted the drop_pyregion branch December 7, 2022 18:48
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: broken optional dependency (pyregion)
3 participants