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: unbounded variable in shear calculation #2850

Merged
merged 5 commits into from
Aug 17, 2020

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Aug 12, 2020

PR Summary

Working on #2848 I opened Pandora's box and found several bugs in shear computation.
This fixes them.
Namely, the function used dimensionality checks where really, what we want is to know wether velocity component are available. The difference is important for AMRVAC datasets, for instance, where one can run a model in a 2D box, but all velocity components are still present.
In particular, if called with 1D data, the intermediate variable f would end up unbounded (which is how I discovered this).

So in summary:

  • correctly include all terms in shear computation for datasets with more velocity components than dimensions (so mostly AMRVAC I think)
  • raise a reasonable error on purpose (ValueError) if called with data that only has 1 velocity component, instead of a defacto UnboundedLocalError
  • Acknowledge the functionality isn't supported yet for non-cartesian data with an NotImplementedError

The reason these changes shouldn't break anything is that the one time this internal function is called is inside a try block that currently catches everything, so introducing early exception raising here is not a problem.

PR Checklist

  • pass black --check yt/
  • pass isort . --check --diff
  • pass flake8 yt/
  • pass flynt yt/ --fail-on-change --dry-run -e yt/extern

@munkm
Copy link
Member

munkm commented Aug 13, 2020

Ok, I'll do my best to review this soon. Would you mind updating the PR description to list (maybe a bulleted list) of all of the bugs that this PR addresses so it's easier to summarize it when we do a release? 🙂

@neutrinoceros
Copy link
Member Author

done !

Copy link
Member

@munkm munkm left a comment

Choose a reason for hiding this comment

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

I think this is nice and I like the specific handling addressed in this PR. I think it would be nice to add more tests for this functionality so we know we're catching things as we expect them. I'll approve this and you can add tests here or open an issue to add more later.

@neutrinoceros
Copy link
Member Author

I'll add tests now and you guys can merge when they pass :)

@neutrinoceros
Copy link
Member Author

Ah I just noticed that the _shear_mach definition is almost an exact duplicate of this one and has the same issues.
I should probably address this here too. Switching to draft for now, I'd like to see if we have tests that will detect if I intentionally break this definition.

@neutrinoceros neutrinoceros marked this pull request as draft August 14, 2020 05:58
@neutrinoceros
Copy link
Member Author

Looks like it's not covered. For the time being, I'll just reapply the same fix there, but I think there's room for a larger code deduplication in this file, that I will attempt to do later.

@neutrinoceros
Copy link
Member Author

for reference, this dataset would be perfect to test some of the edge cases here:
yt-project/website#86

@neutrinoceros neutrinoceros marked this pull request as ready for review August 14, 2020 07:39
@neutrinoceros
Copy link
Member Author

The travis failure looks like another random fail like the one @matthewturk fixed this week. Reported #2861

@munkm munkm merged commit 02faa92 into yt-project:master Aug 17, 2020
@neutrinoceros neutrinoceros deleted the fix_unbounded_variable_shear branch August 17, 2020 19:33
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.

3 participants