Refactors oref to equal the number of zones along each dimension #4037
Conversation
Addresses Issue yt-project#4027
loaders.load_octree() is the most public API that uses over_refine_factor. Changing over_refine_factor to mean the number of zones along each dimension (instead of 2**over_refine_factor) is a breaking change; this reverts that change for load_octree.
|
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution! |
neutrinoceros
added
the
refactor
cphyc
added
enhancement
|
@BolunThompson Thank you very much for doing this! Now that we're actually passing |
|
I've read the changes and I think they are all pretty good. I would like to see the attributes be renamed (and a back-compat shim put in place for now to allow specifying |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
@BolunThompson Let us know when you think this is ready for review? |
|
It should be ready for review now. |
There was a problem hiding this comment.
I think other than the one thing, this looks good to go. I still would like @dnarayanan to take a quick look, though.
| def __init__( | ||
| self, base_region, domain, ds, over_refine_factor=1, num_ghost_zones=0 | ||
| ): | ||
| def __init__(self, base_region, domain, ds, num_zones=2, num_ghost_zones=0): |
There was a problem hiding this comment.
@BolunThompson Could you add a shim here, so that if over_refine_factor is specified it sets num_zones appropriately (and emits a deprecation warning)? We can disable the behavior in 4.2.
|
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
Refactors over_refine_factor and oref to equal the number of zones along each dimension, instead of nz equaling oref to the power of two.
To maintain backwards compatibility (although this seems to be primarily used internally)
loaders.load_octreemaintains the old behavior.Closes #4027