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

hotfix: register "vertex" fields only in unstructured meshes #2848

Merged
merged 5 commits into from Jan 20, 2021

Conversation

neutrinoceros
Copy link
Member

PR Summary

fix #2430

This is a hotfix, I'm convinced there are better ways to solve this but they would require much more work (refactoring).

PR Checklist

  • pass black --check yt/
  • pass isort . --check --diff
  • pass flake8 yt/
  • pass flynt yt/ --fail-on-change --dry-run -e yt/extern
  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

…indexing unstructured-mesh-only fields in other types of datasets
@neutrinoceros
Copy link
Member Author

added a test that fails on master and passes here

@neutrinoceros
Copy link
Member Author

switching to draft since #2850 will make this a tiny bit simpler.

@neutrinoceros neutrinoceros marked this pull request as draft August 12, 2020 21:45
@neutrinoceros
Copy link
Member Author

So I think I'm going to split this between the hotfix and the experimental part where I build a long exception list to avoid except Exception

@neutrinoceros neutrinoceros marked this pull request as ready for review August 13, 2020 06:25
@neutrinoceros
Copy link
Member Author

moved the exception expansion there #2851

@matthewturk
Copy link
Member

Hm, I guess I replied over email but it didn't get through. Anyway, I think there's a possibility we could have vertex-centered fields in some grid calculations. Does this preclude that?

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Aug 13, 2020

I don't think so. My understanding is that vertex fields only make sense for data where ds.index.meshes is defined. If I'm wrong then this needs to be updated of course.
More broadly, I think this is one more case where having specialised abstract Dataset subclasses could help.

@matthewturk
Copy link
Member

@neutrinoceros As long as this does not preclude vertex centered fields (which might be handled with some weird logic at present) for grid fields, this looks good to me and I think it can go in.

@neutrinoceros
Copy link
Member Author

I don't really know how to test this, or how to demonstrate this is fine

@matthewturk
Copy link
Member

matthewturk commented Sep 16, 2020 via email

yt/fields/field_info_container.py Outdated Show resolved Hide resolved
Co-authored-by: Corentin Cadiou <contact@cphyc.me>
@cphyc cphyc merged commit 1223703 into yt-project:master Jan 20, 2021
@neutrinoceros neutrinoceros deleted the bugfix_2430 branch January 20, 2021 12:55
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.

bugs: ds.derived_field_list gets populated with unavailable fields
3 participants