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

Misc changes cherrypicked from actx branch #71

Merged
merged 16 commits into from
Dec 27, 2022

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Nov 19, 2022

Mostly stuff from #56 in an attempt to make that diff smaller!

TODO:

  • Check that refinement works with new fields added to TreeOfBoxes
  • Update docs with new fields added to TreeOfBoxes

@alexfikl
Copy link
Contributor Author

alexfikl commented Nov 19, 2022

The Gitlab CI is running at
https://gitlab.tiker.net/fikl2/boxtree/-/pipelines/362561

boxtree/tree.py Outdated Show resolved Hide resolved
@alexfikl alexfikl force-pushed the tob-updates branch 2 times, most recently from 921583b to b66ac09 Compare November 19, 2022 18:59
test/test_tree_of_boxes.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner

inducer commented Nov 19, 2022

Thanks! This looks good to me outside of the two things we're discussing above.

@inducer
Copy link
Owner

inducer commented Nov 19, 2022

cc @xywei

@inducer
Copy link
Owner

inducer commented Dec 27, 2022

Thanks for working on this. There are still many warts here, but this is at least a step in the right direction.

@inducer
Copy link
Owner

inducer commented Dec 27, 2022

Pushed some changes, LGTM if it passes (I'll work on it if it doesn't).

@inducer inducer enabled auto-merge (rebase) December 27, 2022 00:16
@inducer inducer merged commit 5d9ea31 into inducer:main Dec 27, 2022
@alexfikl alexfikl deleted the tob-updates branch December 27, 2022 07:40
@alexfikl
Copy link
Contributor Author

@inducer Did you take a look if the box_flags and level_start_box_nrs attributes are correct on TreeOfBoxes? I was worried that they don't get updated during refining / coarsening.

@inducer
Copy link
Owner

inducer commented Jan 2, 2023

It seems that refine_and_coarsen_tree_of_boxes make a new TreeOfBoxes that does not have those attributes. Where do you see a risk for incorrect information? Could you be more precise?

@alexfikl
Copy link
Contributor Author

alexfikl commented Jan 3, 2023

It seems that refine_and_coarsen_tree_of_boxes make a new TreeOfBoxes that does not have those attributes. Where do you see a risk for incorrect information? Could you be more precise?

Just from a quick skim in tree_of_boxes.py for box_flags it seems like

and I assume that level_start_box_nrs will need to be updated too if the boxes are moved around so that the starts change?

The functions in there do a replace(tob, ...) so they keep the old versions of those attributes. Should they be set to None instead?

@inducer
Copy link
Owner

inducer commented Jan 5, 2023

_apply_refine_flags_without_sorting makes a new tree from scratch, so I think it's technically OK.

The functions in there do a replace(tob, ...) so they keep the old versions of those attributes. Should they be set to None instead?

I think it'd be even safer to just create a new TreeOfBoxes from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants