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

[WIP] Fix PPVCube #187

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[WIP] Fix PPVCube #187

wants to merge 8 commits into from

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Mar 8, 2023

The PPVCube class is out of date with both yt and yt_astro_analysis. This PR brings it up to date, in particular:

  • Removes deprecated keyword argument depth_res, which was originally here for off-axis projections and is now long gone in yt.
  • Does not assume that the field type will always be "gas".
  • Uses sampling_type="local" in fields to allow for SPH projections.
  • Makes sure field tuples are always used.
  • Removes code used to create line-of-sight velocity fields and uses the built-in yt functionality for this purpose.

Still need to fix the docs, hence WIP. This appears to have uncovered a bug in yt which I'll have to resolve separately.

@brittonsmith
Copy link
Member

@jzuhone, I've looked this over and it all looks reasonable to me. It sounds like we may try to do a new yt_astro release soon. Should we wait for you to finish the docs so we can include this?

@neutrinoceros
Copy link
Member

To be fully transparent there are no real blockers if we want to do a release sooner rather than later. The release process is also smooth enough that we can easily cut one when this goes in, regardless of exactly when it happens :)

@spectram
Copy link
Contributor

@jzuhone Hi, your ppv branch looks great. I am interested in implementing these changes in my branch as well. I am keen to attempt a rebase. Please let me know if that would be helpful to address the conflicts and close this PR.

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.

4 participants