A bugfix for the new expanded EWAH file reading capability #4249
Conversation
|
I probably made a mistake in including the previous PR in a bugfix release. What’s the preferred approach for the backport branch 4.1.x now ? Should we include the fix and hope it’s stable or just revert the original patch there and rerelease everything in 4.2? |
This is a good question for @matthewturk. Either way, this is a critical bugfix so we should fix it somehow in 4.1.3. |
Absolutely |
|
@neutrinoceros actually it's probably easier to just revert the previous change for now so we can think about this more carefully |
done in #4250 |
| # Load Morton index from file if provided | ||
| def _current_fname(order1, order2): | ||
| if getattr(ds, "index_filename", None) is None: | ||
| irange = f"[{order2}-{order2+2}]" |
There was a problem hiding this comment.
It took me a moment to figure out we were creating an fstring for a glob, but this makes sense.
|
@matthewturk what do you think about reverting the feature from the 4.1 branch ? |
|
We did kinda make a big deal of it in the release notes. So if we can fix it, we should, don't you agree? |
|
Sure. I honestly don't hold strong opinions here. This was my mistake, but I'll settle for any solution you guys can agree on |
|
@jzuhone I'm OK with this going in and getting backported. What're your thoughts? |
|
@matthewturk seems like you merged the revert a couple minutes after your last comment. It looks like a mistake on your side this time, but maybe we should actually leave the backport branch alone now, unless anyone feels strongly that we shouldn't ? |
|
I will not touch anything.
…On Fri, Dec 16, 2022 at 11:01 AM Clément Robert ***@***.***> wrote:
@matthewturk <https://github.com/matthewturk> seems like you merged the
revert a couple minutes after your last comment. It looks like a mistake on
your side this time, but maybe we should actually leave the backport branch
alone now, unless anyone feels strongly that we shouldn't ?
—
Reply to this email directly, view it on GitHub
<#4249 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXO3W4UX2EVF46ESU7QTWNSOABANCNFSM6AAAAAAS7JOYSI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Then if @jzuhone agrees I vote for just merging this with no backport, and let future me deal with explaining the revert in the 4.1.3 release notes :-) |
|
@neutrinoceros sounds good to me |
PR Summary
In PR #4198 I implemented a feature to automatically find EWAH files with increased
index_order2for SPH/Lagrangian datasets.There was an oversight in these changes, which involves the fact that although the file with the updated
index_order2is correctly found and loaded,index_order2is not increased by 2 when creating theParticleBitmapobject. The result is that for some analyses that try to look on small scales the code either crashes or it gives odd-looking results.This PR fixes this issue, but it required a bit of moving things around so that we detect the new value of
index_order2from the filename before we create theParticleBitmapobject.PR Checklist