-
Notifications
You must be signed in to change notification settings - Fork 279
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
cell_widths validation for load_uniform_grid #4328
cell_widths validation for load_uniform_grid #4328
Conversation
oh, whoops, looks like I can't use |
There's a discussion to be had on when and how we'll be able to use numpy.typing, but yeah for now the conservative approach is just not to :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the case where nproc > 1
can be improved, but otherwise my suggestions are mostly stylistic, and I'm +1 for improving the user experience with clear data (in)validation !
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Kacper Kowalik <xarthisius.kk@gmail.com>
Summary
This PR:
Background: dtype bug
This started as a bug fix for when the cell_widths do not have a dtype of float64 and expanded a bit to better capture what cell_widths has to be. The bug can be replicated with:
I encountered this while loading some data from a netcdf file where the coordinate arrays happened to be in
float32
. The error that the above code raises is:The bug fix
The fix to the bug is to simply cast the cell_widths to float64 if they are not already. Casting to float64 seems in line with other yt behavior (alternatively I could have caught the case and raised a more meaningful error message than what you get now from deep in the selection routines).
The rest of the PR
In fixing this bug, it seemed a good idea to better encapsulate the requirements for using the
cell_widths
argument withload_uniform_grid
(we are actually missing a docstring entry for cell_widths as well). The summary of those requirements: