ENH: Enable exporting GAMER and FLASH datasets to octrees #4134
Conversation
matthewturk
changed the title
|
close/open to refresh CI now that unrelated failures are resolved on main |
neutrinoceros
added
enhancement
|
Again refreshing CI |
|
@matthewturk Is this still "WIP" or is it actually ready for merging ? |
|
So I think it needs another pass from me. But I will check today! It's currently -8F here with 24MPH winds! Once I get the place under control I'll take a look over this one. Should be a fun task for a funny day. |
|
Best time for a feature freeze!
23 Dec 2022 14:06:10 Matthew Turk ***@***.***>:
… So I think it needs another pass from me. But I will check today!
It's currently -8F here with 24MPH winds! Once I get the place under control I'll take a look over this one. Should be a fun task for a funny day.
—
Reply to this email directly, view it on GitHub[#4134 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABJJII7MDPMH6TG6AKXCCODWOWPUBANCNFSM6AAAAAAQPAPSS4].
You are receiving this because you are subscribed to this thread.[Tracking image][https://github.com/notifications/beacon/ABJJII7BN4QMNPL2TKM6OY3WOWPUBA5CNFSM6AAAAAAQPAPSS6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSRJQGFW.gif]
|
|
Oh, my. That is a hall of fame review comment.
…On Fri, Dec 23, 2022, 7:20 AM Corentin Cadiou ***@***.***> wrote:
Best time for a feature freeze!
23 Dec 2022 14:06:10 Matthew Turk ***@***.***>:
> So I think it needs another pass from me. But I will check today!
>
> It's currently -8F here with 24MPH winds! Once I get the place under
control I'll take a look over this one. Should be a fun task for a funny
day.
>
> —
> Reply to this email directly, view it on GitHub[
#4134 (comment)], or
unsubscribe[
https://github.com/notifications/unsubscribe-auth/ABJJII7MDPMH6TG6AKXCCODWOWPUBANCNFSM6AAAAAAQPAPSS4
].
> You are receiving this because you are subscribed to this
thread.[Tracking image][
https://github.com/notifications/beacon/ABJJII7BN4QMNPL2TKM6OY3WOWPUBA5CNFSM6AAAAAAQPAPSS6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSRJQGFW.gif
]
>
—
Reply to this email directly, view it on GitHub
<#4134 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXO64MZ64JYB6QT4J7FDWOWRKBANCNFSM6AAAAAAQPAPSS4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
matthewturk
changed the title
|
So it's ready for review but I don't want it to be merged quite yet -- a few more minor things I want to address. |
|
looks like this PR grew a couple minor conflicts. I'm also waiting for my comments to be adressed, but it seems like a low-hanging fruit on the journey to yt 4.2 |
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
| # We allow specifying curlevel = -1 to query from the levels array | ||
| # instead. | ||
| if curlevel == -1: | ||
| this_level = levels[p] | ||
| else: | ||
| this_level = curlevel |
There was a problem hiding this comment.
this doesn't depend on the iteration variable (p), so it would be clearer (and more efficient) to put it outside the loop
| @@ -170,7 +170,7 @@ cdef class OctreeContainer: | |||
| pos[2] = self.DLE[2] + dds[2]/2.0 | |||
| for k in range(self.nn[2]): | |||
| if self.root_mesh[i][j][k] == NULL: | |||
| raise RuntimeError | |||
| raise KeyError(i,j,k) | |||
There was a problem hiding this comment.
I guess an IndexError would be slightly more appropriate
There was a problem hiding this comment.
No, I think KeyError is fine. The issue is that we have constructed what amounts to a mapping, and that mapping has nothing in it for a given key.
PR Summary
Initial value: This allows passing in
domain_dimensionsto the load_octree function, so that you can load octrees that don't have just the one root cell.New and revised: This PR will enable FLASH and GAMER datasets to be exposed as Octree datasets, which will be a method for testing improvements in performance.
PR Checklist