Fix: Bounds in output for Metadata::None variables #1188

AstroBarker · 2024-10-10T21:59:50Z

In updating phoebus to use up-to-date parthenon I ran into an issue where a Metadata::None variable was failing to output due to view bounds accesses. There seems to have been an off-by-one error in the relevant code block in output_utils.hpp:313. I've updated the bounds there to correctly output Metadata::None variables.

To test this, I modified the advection example to have an optional Metadata::None variable (enabled in the input deck <Advection>/test_metadata_none). I set the output_hdf5 test to use this variable.

PR Checklist

…dd to hdf5 regression tests

pdmullen

Nice find! Output bugs are always tricky to track down.

Yurlungur

Thanks for the fix! This breakage is definitely on me.

Yurlungur · 2024-10-14T17:05:14Z

Looks like tests are failing @AstroBarker ?

AstroBarker · 2024-10-14T17:08:01Z

Looks like tests are failing @AstroBarker ?

Looks like output_hdf5 so probably it's the gold files

Yurlungur · 2024-10-14T17:09:10Z

almost certainly. run locally and see what the error is. might need to update the gold files.

AstroBarker · 2024-10-14T17:12:40Z

almost certainly. run locally and see what the error is. might need to update the gold files.

Yeah they pass locally for me only if I update the goldfiles. I added a new output for the Metadata::None variable in the hdf5 tests. What's the process for updating the proper gold files?

Yurlungur · 2024-10-14T17:15:06Z

Run parthenon/tst/regression/gold_standard/make_tarball.sh. Then create a release from the generated tarball. You will also need to update the tarball version and hash in the CMake

pgrete

In addition to the comment on filling the content of the new var: Should we also add a proper check that this is working, i.e., read the data of the variable to confirm that the data written is looking what it's supposed to look like?

pgrete · 2024-10-17T08:40:54Z

example/advection/advection_package.cpp

+  if (test_metadata_none) {
+    IndexRange ib = pmb->cellbounds.GetBoundsI(IndexDomain::interior);
+    IndexRange jb = pmb->cellbounds.GetBoundsJ(IndexDomain::interior);
+    IndexRange kb = pmb->cellbounds.GetBoundsK(IndexDomain::interior);
+
+    // packing in principle unnecessary/convoluted here and just done for demonstration
+    std::vector<std::string> vars({"metadata_none_var"});
+    PackIndexMap imap;
+    const auto &v = rc->PackVariables(vars, imap);
+
+    const int ivar = imap.get("metadata_none_var").first;
+    pmb->par_for(
+        PARTHENON_AUTO_LABEL, 0, 2, kb.s, kb.e, jb.s, jb.e, ib.s, ib.e,
+        KOKKOS_LAMBDA(const int n, const int k, const int j, const int i) {
+          v(ivar, n, k, j, i) = n + k * j * i;
+        });
+  }


I'm not sure what/why this is happening here.
If I read the the field correctly, then it's a vector with 3 components and extents larger than the mesh size by one.
In this par_for the block indices are used (without any global offset), so every block will have the same indices set to the same values and a large fraction of the index space will remain 0.
Is that intentional?

Good catch, I didn't pay much mind to the contents of the fields, only that its shape reproduced the crash. This should be fixed to be filled now, I think.

…ate golds.

pgrete · 2024-10-29T10:49:35Z

example/advection/advection_package.cpp

+          v(ivar_lo, n, k, j, i) = n + k * j * i;
+          v(ivar_lo + 1, n, k, j, i) = 1 + n + k * j * i;
+          v(ivar_hi, n, k, j, i) = 2 + n + k * j * i;


i just looked at the updated loop.
While I understand the fix going from mesh to meshbock, I now don't get the ivar_lo/ivar_hi indexing.
What's supposed to happen here? I could see this working if ivar_lo/ivar_hi replace the n` index.

Any comment on this? ping @AstroBarker

I'm happy to merge this without adding test to read the data (which would be great), but I'd like to understand this loop before merging as from my current understanding it might seem like being an unintended bug.

I've updated the loop, removing the ivar_lo/hi stuff, which I agree was not necessary in the loop, and added a section to the hdf5 test that checks the contents of this variable. Sorry for the delay!

Great, thanks!

tst/regression/test_suites/output_hdf5/parthinput.advection

pgrete · 2024-11-13T15:02:25Z

Formatter does not work on forks. I'm force merging.

pgrete · 2024-11-13T15:03:57Z

I don't see the force merge button any more? Am I missing sth?

Yurlungur · 2024-11-13T15:09:38Z

I don't see the force merge button any more? Am I missing sth?

I already clicked auto merge. It seems like not all the "long" tests triggered. I will try to disable/re-enable.

pgrete · 2024-11-13T15:18:42Z

I don't see the force merge button any more? Am I missing sth?

I already clicked auto merge. It seems like not all the "long" tests triggered. I will try to disable/re-enable.

Yes, that's what I did earlier today and all tests passed (except for the formatting one that, as we know, does not report results for PRs from forks).
Previously, there was a force merge button but I don't see it any more (maybe because of the changed branched protection rules?)

Yurlungur · 2024-11-13T15:35:50Z

I don't see the force merge button any more? Am I missing sth?

I already clicked auto merge. It seems like not all the "long" tests triggered. I will try to disable/re-enable.

Yes, that's what I did earlier today and all tests passed (except for the formatting one that, as we know, does not report results for PRs from forks). Previously, there was a force merge button but I don't see it any more (maybe because of the changed branched protection rules?)

If you want to force merge, disable auto-merge. Then there should be an option.

pgrete · 2024-11-13T15:45:31Z

I don't see the force merge button any more? Am I missing sth?

I already clicked auto merge. It seems like not all the "long" tests triggered. I will try to disable/re-enable.

Yes, that's what I did earlier today and all tests passed (except for the formatting one that, as we know, does not report results for PRs from forks). Previously, there was a force merge button but I don't see it any more (maybe because of the changed branched protection rules?)

If you want to force merge, disable auto-merge. Then there should be an option.

That is basically my question. I don't see the button any more.
So if you still see it, feel free to go ahead and force merge (as the tests including extended ones passed earlier today).

Yurlungur · 2024-11-13T15:49:54Z

I don't see the force merge button any more? Am I missing sth?

I already clicked auto merge. It seems like not all the "long" tests triggered. I will try to disable/re-enable.

Yes, that's what I did earlier today and all tests passed (except for the formatting one that, as we know, does not report results for PRs from forks). Previously, there was a force merge button but I don't see it any more (maybe because of the changed branched protection rules?)

If you want to force merge, disable auto-merge. Then there should be an option.

That is basically my question. I don't see the button any more. So if you still see it, feel free to go ahead and force merge (as the tests including extended ones passed earlier today).

Shoot you're right. I don't see it either. Maybe we submit a new MR on a branch and see if that goes through? Should figure out how to make fork merges work though...

pgrete · 2024-11-13T15:56:18Z

Alright, so it wasn't just me and it was related to the branch protection rules.
I recently enabled

because of my accidental push to develop.
So I temp disabled it, now force merged, and then re-enabled it as a quick workaround for this PR.

In the long run it'd definitely be nice to have the formatter also trigger for forks.
I looked into this in the past and was not able to figure out a solution within a reasonable time (and thus tabled it).
So from my point of view, I'm happy to continue working with a manual work around (unless more and more PRs are coming from forks) and spent the time debugging CI on other (CI) pieces ;)

Yurlungur · 2024-11-13T15:59:42Z

Aah I see. Ok sounds good.

AstroBarker added 2 commits October 10, 2024 21:11

fix: metadata::none output bounds

5ed2a20

tst: add option in advection example to test metadata::none output. A…

af6714e

…dd to hdf5 regression tests

pdmullen approved these changes Oct 10, 2024

View reviewed changes

Yurlungur enabled auto-merge October 10, 2024 23:04

Yurlungur approved these changes Oct 10, 2024

View reviewed changes

changelog

9ac4e83

auto-merge was automatically disabled October 15, 2024 02:16
Head branch was pushed to by a user without write access

gold file SHA

73a32eb

pgrete reviewed Oct 17, 2024

View reviewed changes

AstroBarker and others added 2 commits October 21, 2024 15:32

update metadata_none_Var in advection example to fit block sizes. upd…

1fbb058

…ate golds.

Merge branch 'develop' into develop

20abe85

Yurlungur enabled auto-merge October 24, 2024 22:33

pgrete reviewed Oct 29, 2024

View reviewed changes

pgrete disabled auto-merge November 12, 2024 15:53

AstroBarker added 2 commits November 12, 2024 16:12

updates to metadata none var test

a064a26

update SHA for golds

38e4e60

pgrete approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'develop' into develop

71f3668

pgrete enabled auto-merge (squash) November 13, 2024 09:54

pgrete disabled auto-merge November 13, 2024 15:02

pgrete enabled auto-merge (squash) November 13, 2024 15:03

Yurlungur disabled auto-merge November 13, 2024 15:09

Yurlungur enabled auto-merge November 13, 2024 15:09

Yurlungur disabled auto-merge November 13, 2024 15:47

Yurlungur enabled auto-merge November 13, 2024 15:48

Yurlungur disabled auto-merge November 13, 2024 15:51

Yurlungur enabled auto-merge November 13, 2024 15:51

pgrete disabled auto-merge November 13, 2024 15:53

pgrete merged commit b3a2b8d into parthenon-hpc-lab:develop Nov 13, 2024
43 of 58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Bounds in output for Metadata::None variables #1188

Fix: Bounds in output for Metadata::None variables #1188

AstroBarker commented Oct 10, 2024

pdmullen left a comment

Yurlungur left a comment

Yurlungur commented Oct 14, 2024

AstroBarker commented Oct 14, 2024

Yurlungur commented Oct 14, 2024

AstroBarker commented Oct 14, 2024

Yurlungur commented Oct 14, 2024

pgrete left a comment

pgrete Oct 17, 2024

AstroBarker Oct 21, 2024 •

edited

Loading

pgrete Oct 29, 2024

pgrete Nov 12, 2024

AstroBarker Nov 12, 2024

pgrete Nov 13, 2024

pgrete commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024 •

edited

Loading

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

Fix: Bounds in output for Metadata::None variables #1188

Fix: Bounds in output for Metadata::None variables #1188

Conversation

AstroBarker commented Oct 10, 2024

PR Checklist

pdmullen left a comment

Choose a reason for hiding this comment

Yurlungur left a comment

Choose a reason for hiding this comment

Yurlungur commented Oct 14, 2024

AstroBarker commented Oct 14, 2024

Yurlungur commented Oct 14, 2024

AstroBarker commented Oct 14, 2024

Yurlungur commented Oct 14, 2024

pgrete left a comment

Choose a reason for hiding this comment

pgrete Oct 17, 2024

Choose a reason for hiding this comment

AstroBarker Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

pgrete Oct 29, 2024

Choose a reason for hiding this comment

pgrete Nov 12, 2024

Choose a reason for hiding this comment

AstroBarker Nov 12, 2024

Choose a reason for hiding this comment

pgrete Nov 13, 2024

Choose a reason for hiding this comment

pgrete commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024 • edited Loading

Yurlungur commented Nov 13, 2024

pgrete commented Nov 13, 2024

Yurlungur commented Nov 13, 2024

AstroBarker Oct 21, 2024 •

edited

Loading

pgrete commented Nov 13, 2024 •

edited

Loading