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 mistake in call to partial function #3081

Merged
merged 8 commits into from
Apr 8, 2021

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Feb 22, 2021

PR Summary

This bug was reported by @claytonstrawn on our channel. It is due to a minor typo in the way art deals with particle velocity fields which caused a Python error.

Steps to reproduce:

import yt
ds = yt.load_sample("D9p_500")
ad = ds.all_data()
ad["particle_velocity_x"]

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@cphyc cphyc added the bug label Feb 22, 2021
@cphyc
Copy link
Member Author

cphyc commented Feb 22, 2021

At the moment, the test(s) are not triggered at all. I'm working on it but in the meantime I'm switching to draft.

@cphyc cphyc marked this pull request as ready for review February 24, 2021 14:34
@cphyc
Copy link
Member Author

cphyc commented Feb 24, 2021

@matthewturk @brittonsmith @neutrinoceros this is ready to go (I just need to bump the answer tests for Jenkins). However, it seems #3087 would require bumping the answer tests again. My question is thus: should we fix #3087 in this PR before merging or submit the answer tests knowing the velocities may be off?

@matthewturk
Copy link
Member

I think whatever gets this in and closed is best.

@neutrinoceros
Copy link
Member

We lost the report, I need to rerun tests
@yt-fido test this please

@munkm
Copy link
Member

munkm commented Mar 8, 2021

I think it's fine to not fix #3087 in this PR. We can do a follow up PR for that, and now that we have the submodule for answers I don't think it's a huge issue to have lots of answer-store commits. Unless I'm missing something?

@neutrinoceros
Copy link
Member

I don't think it's a huge issue to have lots of answer-store commits. Unless I'm missing something?

FWIW, I think it's already hard enough to properly handle a single PR there at a time, I don't think anyone wants to abuse them if it can be avoided. But if we need 2 updates instead of one, that's fine by me :)

@munkm munkm merged commit 212bcdb into yt-project:main Apr 8, 2021
@cphyc cphyc deleted the fix/art-error-with-particle-velocity branch October 19, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants