[breaking] output more topological elements #1019

Yurlungur · 2024-03-11T22:21:41Z

PR Summary

What it says on the can

PR Checklist

bprather · 2024-03-11T22:27:08Z

So many feature PRs with net negative lines! Approach looks great, can test/debug later this week. This is a super important piece of putting KHARMA back on Parthenon develop. I'll have another interesting little piece to push upstream in the next week too, and then I can stop frankenbranching! Exciting.

Yurlungur · 2024-03-11T22:34:25Z

I'm not done yet. This is currently just cleanup. But the feature will be in soon. Hopefully before you look. :)

pgrete

Just had quick look at the PR following our discussion on Thursday and it looks like a nice cleanup (though I didn't review the code in great detail yet).
Note, that I'm about to make the RestartReader a base class so that I can derive for hdf5 and OpenPMD.
Do you expect to touch the restart parts (beyond the removed legacy code)?

Yurlungur · 2024-03-18T17:08:38Z

Just had quick look at the PR following our discussion on Thursday and it looks like a nice cleanup (though I didn't review the code in great detail yet). Note, that I'm about to make the RestartReader a base class so that I can derive for hdf5 and OpenPMD. Do you expect to touch the restart parts (beyond the removed legacy code)?

I will need to touch the utility functions called by the restart reader, such as the pack/unpack variables. I will also need to change ReadBlocks in restart.hpp.

pgrete · 2024-03-18T19:32:02Z

Just had quick look at the PR following our discussion on Thursday and it looks like a nice cleanup (though I didn't review the code in great detail yet). Note, that I'm about to make the RestartReader a base class so that I can derive for hdf5 and OpenPMD. Do you expect to touch the restart parts (beyond the removed legacy code)?

I will need to touch the utility functions called by the restart reader, such as the pack/unpack variables. I will also need to change ReadBlocks in restart.hpp.

Right, but this is all going to be "internal" to restarting from hdf5, correct?
The OpenPMD logic will a little bit different, so I'm mostly interested if you expect changes to the interface that's being called (not what happens within the functions themselves).

Yurlungur · 2024-03-18T19:34:30Z

Just had quick look at the PR following our discussion on Thursday and it looks like a nice cleanup (though I didn't review the code in great detail yet). Note, that I'm about to make the RestartReader a base class so that I can derive for hdf5 and OpenPMD. Do you expect to touch the restart parts (beyond the removed legacy code)?

I will need to touch the utility functions called by the restart reader, such as the pack/unpack variables. I will also need to change ReadBlocks in restart.hpp.

Right, but this is all going to be "internal" to restarting from hdf5, correct? The OpenPMD logic will a little bit different, so I'm mostly interested if you expect changes to the interface that's being called (not what happens within the functions themselves).

Ah I see... I'm not sure. I was thinking of trying to loop the VarInfo through reading restarts too, to re-use that code... but it might be more trouble than it's worth.

…mm/output-nodes-2

pgrete · 2024-03-18T20:18:25Z

Just had quick look at the PR following our discussion on Thursday and it looks like a nice cleanup (though I didn't review the code in great detail yet). Note, that I'm about to make the RestartReader a base class so that I can derive for hdf5 and OpenPMD. Do you expect to touch the restart parts (beyond the removed legacy code)?

I will need to touch the utility functions called by the restart reader, such as the pack/unpack variables. I will also need to change ReadBlocks in restart.hpp.

Right, but this is all going to be "internal" to restarting from hdf5, correct? The OpenPMD logic will a little bit different, so I'm mostly interested if you expect changes to the interface that's being called (not what happens within the functions themselves).

Ah I see... I'm not sure. I was thinking of trying to loop the VarInfo through reading restarts too, to re-use that code... but it might be more trouble than it's worth.

Alright, as far as I can tell right now my refactor won't be that intrusive, so I'll just go ahead and then we can mix/match later (especially as I'll probably use more from the change you implement here than the other way around).

Yurlungur · 2024-03-28T18:34:16Z

OK this is now ready for merge after sufficient approval/testing. Plotted below is the z-component of the vector potential for a rotor problem.

Yurlungur · 2024-03-29T00:28:23Z

@pgrete @bprather this is fully ready for merge with 1 more review.

src/outputs/output_utils.cpp

Co-authored-by: Adam M. Dempsey <[email protected]>

pgrete

Quite a rabbit hole, which came out nicely clean wrt to the changes to the hdf5 piece alone.
I'm done with the code review (with a couple of minor comments), and will approve tomorrow once I confirmed things are working (particularly restarting version 3 outputs) in AthenaPK.

pgrete · 2024-04-01T19:11:03Z

src/interface/variable.hpp

@@ -212,7 +217,7 @@ class ParticleVariable {

  KOKKOS_FORCEINLINE_FUNCTION
  auto GetDim(const int i) const {
-    PARTHENON_DEBUG_REQUIRE(0 < i && i <= 6, "ParArrayNDGenerics are max 6D");
+    PARTHENON_DEBUG_REQUIRE(0 < i && i <= 6, "Index out of bounds");


But it's also std::array<int, MAX_VARIABLE_DIMENSION> dims_; below, isn't it?

pgrete · 2024-04-01T19:14:06Z

scripts/python/packages/parthenon_tools/parthenon_tools/movie2d.py

+            print("value error!", len(components), ntensors, q.shape)
+            raise ValueError(
+                "Tensor rank not the same as number of specified components"


Any reason for the separate preceding print statement rather than putting everything in the raised error message?

The exceptions raised within a thread (since the code is thread-parallel) were not being propagated, so the script was just failing silently. This was my hacky way of dealing with it.

I just went back in and fixed it properly, by forcing python to raise exceptions.

scripts/python/packages/parthenon_tools/parthenon_tools/movie2d.py

pgrete · 2024-04-01T19:18:29Z

scripts/python/packages/parthenon_tools/parthenon_tools/movie2d.py

+        for c in components:
+            if c > (q.shape[1] - 1):


What's exactly happening here (in combination with the q = q[:, c] below.
The for c in ... make me believe there's more than one component, but then it's checked against a fixed index in q.

the intent here is that q always has the shape [blocks, some number of tensor components, k, j, i] so q[1] is always the index of the slowest-moving tensor component. Then q = q[:,c] sets the slowest moving tensor-component to c but leaves the block index free floating. If you iterate with this, q keeps changing shape, so you can walk throw all tensor components, slowest first. I added a comment in the code to explain.

pgrete · 2024-04-01T19:25:06Z

scripts/python/packages/parthenon_tools/parthenon_tools/phdf.py

+           If variable is a vector, each element of the returned numpy
+           array is a vector of that length.


I'm having trouble parsing that statement. Does "length" here refer to the flattened length, i.e. you get a 2D numpy array back with [c,TotalCells] with c being the vector.
In that case, how about moving this statement after the first sentence of the following paragraph as (so that there's an explanation for "length".

I don't think I wrote this line. I just moved it/reformated it. I also don't understand what it means. I've deleted it and modified the passage below it to explain.

src/outputs/restart_hdf5.cpp

src/parthenon_manager.cpp

tst/unit/test_output_utils.cpp

pgrete

Thanks for the updates.
I also just verified that things work in AthenaPK.
Ready to merge!

jonahm-LANL added 13 commits March 9, 2024 19:15

deprecate older file formats.

74b1278

get rid of some of those hardcoded arrays...

fd7ad48

remove more hardcoded initializer lists

7bb8c08

update error messages

429260a

some more careful initialization without hardcoding

061c0bf

remove some unused vars

bfe5427

unused var

a3f3469

remove more hardcoded stuff

243d94e

TensorSize

1864e79

switch to using an array in VarInfo

732500f

formatting

534e38b

lint

41a9db2

Merge branch 'develop' into jmm/yet-more-io-cleanup

e5e35f0

Yurlungur requested a review from bprather March 11, 2024 22:21

Yurlungur self-assigned this Mar 11, 2024

jonahm-LANL added 5 commits March 13, 2024 08:50

generalize how shapes are set

4f12e29

remove prefilling

e87f598

move FillShape

d802f4b

clean up compression

5d49b49

extend dim finally

e994a31

pgrete reviewed Mar 18, 2024

View reviewed changes

Merge branch 'develop' into jmm/output-nodes-2

fcf37ea

jonahm-LANL added 2 commits March 18, 2024 14:00

add unit tests... more to come

a45f5ef

Merge branch 'jmm/output-nodes-2' of github.com:lanl/parthenon into j…

072fd30

…mm/output-nodes-2

Yurlungur changed the base branch from pgrete/refactor-restart to develop March 28, 2024 18:17

Yurlungur changed the title ~~output more topological elements~~ [breaking] output more topological elements Mar 28, 2024

jonahm-LANL added 3 commits March 28, 2024 12:19

changelog for breaking, update CC

c87de36

merge develop

3e3154c

disable vector tag in xdmf if not cell-centered

a68a782

jonahm-LANL added 6 commits March 28, 2024 12:40

actually short-circuit special casing for vector logic

27a580f

caveat edge locations

21b4a9d

warning

45a896d

more verbose error in phdf

492cb70

stupid python UTF-8 nonsense

242207f

reset nghost to not break other unit tests

ff1c609

Yurlungur enabled auto-merge March 29, 2024 00:27

get ascent to behave itself

d5aba21

Yurlungur disabled auto-merge March 29, 2024 15:49

Yurlungur enabled auto-merge March 29, 2024 15:49

Yurlungur mentioned this pull request Mar 29, 2024

custom coords #1039

Merged

12 tasks

adamdempsey90 reviewed Mar 30, 2024

View reviewed changes

src/outputs/output_utils.cpp Outdated Show resolved Hide resolved

Update src/outputs/output_utils.cpp

3f9fb86

Co-authored-by: Adam M. Dempsey <[email protected]>

pgrete reviewed Apr 1, 2024

View reviewed changes

fix exceptions

62bce90

Yurlungur mentioned this pull request Apr 1, 2024

Test non-uniform block shapes #1041

Open

pgrete comments

5d91dd9

pgrete approved these changes Apr 3, 2024

View reviewed changes

Merge branch 'develop' into jmm/output-nodes-2

9c904b6

Yurlungur disabled auto-merge April 3, 2024 16:49

Yurlungur enabled auto-merge April 3, 2024 16:49

Yurlungur merged commit b9ed698 into develop Apr 3, 2024
49 checks passed

Yurlungur deleted the jmm/output-nodes-2 branch April 3, 2024 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breaking] output more topological elements #1019

[breaking] output more topological elements #1019

Yurlungur commented Mar 11, 2024 •

edited

Loading

bprather commented Mar 11, 2024

Yurlungur commented Mar 11, 2024

pgrete left a comment

Yurlungur commented Mar 18, 2024

pgrete commented Mar 18, 2024

Yurlungur commented Mar 18, 2024

pgrete commented Mar 18, 2024

Yurlungur commented Mar 28, 2024

Yurlungur commented Mar 29, 2024

pgrete left a comment

pgrete Apr 1, 2024

pgrete Apr 1, 2024

Yurlungur Apr 1, 2024

pgrete Apr 1, 2024

Yurlungur Apr 1, 2024

pgrete Apr 1, 2024

Yurlungur Apr 1, 2024

pgrete left a comment

		If variable is a vector, each element of the returned numpy
		array is a vector of that length.

[breaking] output more topological elements #1019

[breaking] output more topological elements #1019

Conversation

Yurlungur commented Mar 11, 2024 • edited Loading

PR Summary

PR Checklist

bprather commented Mar 11, 2024

Yurlungur commented Mar 11, 2024

pgrete left a comment

Choose a reason for hiding this comment

Yurlungur commented Mar 18, 2024

pgrete commented Mar 18, 2024

Yurlungur commented Mar 18, 2024

pgrete commented Mar 18, 2024

Yurlungur commented Mar 28, 2024

Yurlungur commented Mar 29, 2024

pgrete left a comment

Choose a reason for hiding this comment

pgrete Apr 1, 2024

Choose a reason for hiding this comment

pgrete Apr 1, 2024

Choose a reason for hiding this comment

Yurlungur Apr 1, 2024

Choose a reason for hiding this comment

pgrete Apr 1, 2024

Choose a reason for hiding this comment

Yurlungur Apr 1, 2024

Choose a reason for hiding this comment

pgrete Apr 1, 2024

Choose a reason for hiding this comment

Yurlungur Apr 1, 2024

Choose a reason for hiding this comment

pgrete left a comment

Choose a reason for hiding this comment

Yurlungur commented Mar 11, 2024 •

edited

Loading