[Breaking change] Add fine field option #991

lroberts36 · 2023-12-19T01:35:32Z

PR Summary

This PR adds the new flag Metadata::Fine, which indicates that the field should have twice the resolution of default fields. Fields with this flag can exchange ghosts, and be prolongated and restricted similarly to normal fields. They can also be output, although none of the plotting scripts have been updated to deal with fine fields. This feature is in use in Assail.

PR Checklist

Yurlungur

Well this was remarkably simple. I was expecting it to be a little more complicated than this but nice to see it worked out so easily.

example/advection/advection_package.cpp

lroberts36 · 2023-12-19T01:51:53Z

Well this was remarkably simple. I was expecting it to be a little more complicated than this but nice to see it worked out so easily.

@Yurlungur: Well, I have only tested for unigrid so not totally sure it works for prolongation and restriction. But at least it runs with AMR w/o segfaults.

Yurlungur · 2023-12-19T01:54:55Z

Well this was remarkably simple. I was expecting it to be a little more complicated than this but nice to see it worked out so easily.

@Yurlungur: Well, I have only tested for unigrid so not totally sure it works for prolongation and restriction. But at least it runs with AMR w/o segfaults.

I suppose we should check for correctness with AMR lol.

adamdempsey90

Downstream app looks good

doc/sphinx/src/interface/metadata.rst

Co-authored-by: Adam M. Dempsey <adamdemps@gmail.com>

pgrete

Just did a quick review. I understand where this feature might be useful and just have a couple of comments regarding the current implementation.

doc/sphinx/src/interface/metadata.rst

example/advection/advection_driver.cpp

example/advection/advection_package.cpp

src/bvals/comms/bnd_info.cpp

src/outputs/output_utils.cpp

src/mesh/meshblock.cpp

Yurlungur

I like the new example!

example/fine_advection/advection_driver.cpp

example/fine_advection/advection_package.hpp

example/fine_advection/parthenon_app_inputs.cpp

example/fine_advection/stokes.hpp

Yurlungur · 2024-05-22T17:17:11Z

src/coordinates/uniform_cartesian.hpp

+    if (cl == CellLevel::same) {
+      if (el == TE::CC) {
+        return cell_volume_;
+      } else if (el == TE::F1) {
+        return area_[X1DIR - 1];
+      } else if (el == TE::F2) {
+        return area_[X2DIR - 1];
+      } else if (el == TE::F3) {
+        return area_[X3DIR - 1];
+      } else if (el == TE::E1) {
+        return dx_[X1DIR - 1];
+      } else if (el == TE::E2) {
+        return dx_[X2DIR - 1];
+      } else if (el == TE::E3) {
+        return dx_[X3DIR - 1];
+      } else if (el == TE::NN) {
+        return 1.0;
+      }


Is this all new or can we call the older volume method which defaults to CellLevel::same?

the old Volume method is templated on the topological element. Here I added a new Volume overload that is not templated and takes a topological type and a cell level as an argument. It should give the same results as the templated volume function.

What I meant was that for the CellLevel::same branch can you call the old method?

You could, but since the old one is templated on TE and this one is not it doesn't save any lines of code since you would have to call each template by hand. I am not actually sure that this really needs to be templates though.

Oh I see. Could the old one optionally take a runtime? That might be useful...

Yeah, I think the best solution might be to have the old one just pass the template argument as a regular argument to the new one and have the new one have a default argument of cell level same. I doubt that has any performance impact.

src/interface/sparse_pack.hpp

src/mesh/mesh.cpp

pgrete

LGTM
Thanks for putting the extra effort in to make it (almost) feature complete.
I just got a few minor comments.

example/fine_advection/CMakeLists.txt

pgrete · 2024-06-13T13:41:46Z

example/fine_advection/advection_driver.cpp

+    auto start_send = tl.AddTask(none, parthenon::StartReceiveBoundaryBuffers, mc1);
+    auto start_flxcor = tl.AddTask(none, parthenon::StartReceiveFluxCorrections, mc0);


Independent of this PR but relevant in the context of recent discussion around exhausting MPI limits, it might be worth to change the pattern here to first start all receives (in their own TaskRegion) rather than starting them "per pack" as the behavior will then depend on the pack size.

Yeah, that seems reasonable.

example/fine_advection/advection_driver.cpp

example/fine_advection/parthinput.advection

example/fine_advection/advection_driver.cpp

example/fine_advection/stokes.hpp

src/bvals/boundary_conditions_generic.hpp

pgrete · 2024-06-13T14:18:43Z

src/bvals/boundary_conditions_generic.hpp

+    pmb->par_for_bndry(
+        PARTHENON_AUTO_LABEL, nb, domain, el, coarse, fine,
+        KOKKOS_LAMBDA(const int &l, const int &k, const int &j, const int &i) {
+          if (TYPE == BCType::Reflect) {
+            const bool reflect = (q(b, el, l).vector_component == DIR);
+            q(b, el, l, k, j, i) =
+                (reflect ? -1.0 : 1.0) * q(b, el, l, X3 ? offset - k : k,
+                                           X2 ? offset - j : j, X1 ? offset - i : i);
+          } else if (TYPE == BCType::FixedFace) {
+            q(b, el, l, k, j, i) =
+                2.0 * val - q(b, el, l, X3 ? offset - k : k, X2 ? offset - j : j,
+                              X1 ? offset - i : i);
+          } else if (TYPE == BCType::ConstantDeriv) {
+            Real dq = q(b, el, l, X3 ? ref + offsetin : k, X2 ? ref + offsetin : j,
+                        X1 ? ref + offsetin : i) -
+                      q(b, el, l, X3 ? ref - offsetout : k, X2 ? ref - offsetout : j,
+                        X1 ? ref - offsetout : i);
+            Real delta = 0.0;


Given the complexity of this code (and above in this file) is it worth to update the new test case for use of non-periodic boundary conditions?

yes, I think we want test coverage for all types of boundary conditions, different forest configurations, sparse and non-sparse, and regular and fine fields. Once the rest of these PRs that rely on this one are in, I was going to make another PR adding regression tests, add gold files, and maybe remove the old sparse advection test.

pgrete · 2024-06-13T14:22:43Z

src/coordinates/uniform_cartesian.hpp

+      }
+    } else if (cl == CellLevel::fine) {
+      if (el == TE::CC) {
+        return cell_volume_ / 8.0;


Is this generally true (i.e., design decision) for <3D sims?
I don't recall how we handle this right now.

I think that how we deal with, e.g., 2D simulations is that they are actually 3D (so the volume of a cell is a 3-volume) but we just assume they have translational symmetry in the z-direction. In that case, I think dividing by 8 here for all ndim is the right thing to do.

pgrete · 2024-06-13T14:29:45Z

tst/regression/test_suites/restart_fine/parthinput.restart_fine

+#  Athena++ astrophysical MHD code
+#  Copyright(C) 2014 James M. Stone <jmstone@princeton.edu> and other code contributors
+#  Licensed under the 3-clause BSD License, see LICENSE file for details


-> Parthenon

BenWibking · 2024-06-18T19:14:05Z

This broke the existing use of pmb->par_for_bndry(...) in AthenaPK. It looks like this was missed from the "breaking changes" section of the changelog.

If we are not using fine fields, should the fine argument of par_for_bndry always be false?

lroberts36 · 2024-06-18T19:20:01Z

@BenWibking: Yes, the fine argument should just be set to false. Sorry, I included the breaks-downstream label but I guess forgot to put this in the correct section of changelog.

Seemingly working fine fields, w/o boundary conditions

63a02c4

lroberts36 requested a review from Yurlungur December 19, 2023 01:35

format and lint

23bb48c

Yurlungur approved these changes Dec 19, 2023

View reviewed changes

example/advection/advection_package.cpp Outdated Show resolved Hide resolved

example/advection/advection_package.cpp Outdated Show resolved Hide resolved

lroberts36 added 12 commits April 11, 2024 09:44

Merge branch 'develop' into lroberts36/add-fine-variables

17cc7a5

Merge branch 'develop' into lroberts36/add-fine-variables

96157a8

small

4f55a57

format

d6b3b8d

Merge branch 'develop' into lroberts36/add-fine-variables

e1c7765

Remove printout

4f85d41

remove comment

72d298b

enable fine field output and restart

88b0265

format

82d1ab0

Remove unused tasks

bd628c9

document flag

bbf64b6

fix tests

809c002

lroberts36 changed the title ~~WIP: Add fine field option~~ Add fine field option May 14, 2024

lroberts36 requested review from jdolence, pdmullen, pgrete and Yurlungur May 14, 2024 00:18

Yurlungur approved these changes May 14, 2024

View reviewed changes

lroberts36 requested a review from adamdempsey90 May 14, 2024 01:14

Merge branch 'develop' into lroberts36/add-fine-variables

feba54c

adamdempsey90 approved these changes May 15, 2024

View reviewed changes

doc/sphinx/src/interface/metadata.rst Outdated Show resolved Hide resolved

Update doc/sphinx/src/interface/metadata.rst

4de848d

Co-authored-by: Adam M. Dempsey <adamdemps@gmail.com>

pgrete reviewed May 15, 2024

View reviewed changes

Respond to Philipps comments

8b2bbeb

Merge branch 'develop' into lroberts36/add-fine-variables

247fc59

Yurlungur approved these changes May 22, 2024

View reviewed changes

lroberts36 added 2 commits May 22, 2024 17:27

Merge branch 'develop' into lroberts36/add-fine-variables

0f07b70

Merge branch 'develop' into lroberts36/add-fine-variables

27d3676

lroberts36 mentioned this pull request May 29, 2024

Updates to DataCollection #1092

Merged

13 tasks

lroberts36 and others added 6 commits May 29, 2024 13:45

Merge branch 'develop' into lroberts36/add-fine-variables

afccac5

Merge branch 'develop' into lroberts36/add-fine-variables

a4bedd6

Merge branch 'develop' into lroberts36/add-fine-variables

7245148

Merge branch 'develop' into lroberts36/add-fine-variables

30d57ee

Merge branch 'develop' into lroberts36/add-fine-variables

90ff0dc

Merge branch 'develop' into lroberts36/add-fine-variables

8982895

pgrete approved these changes Jun 13, 2024

View reviewed changes

lroberts36 added 8 commits June 13, 2024 11:55

fix copyright

cf28375

add comment

7e32ab3

Add a README

593564d

copyright

e439478

remove unused

50711d6

check bounds

fdc437e

format

4362d9d

Merge branch 'develop' into lroberts36/add-fine-variables

16bc508

lroberts36 enabled auto-merge June 13, 2024 18:11

bugfix

e4a63b4

lroberts36 disabled auto-merge June 13, 2024 18:25

lroberts36 enabled auto-merge June 13, 2024 18:25

lroberts36 disabled auto-merge June 13, 2024 19:41

lroberts36 enabled auto-merge June 13, 2024 19:42

lroberts36 merged commit f3916be into develop Jun 13, 2024
51 of 54 checks passed

BenWibking changed the title ~~Add fine field option~~ [Breaking change] Add fine field option Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Breaking change] Add fine field option #991

[Breaking change] Add fine field option #991

lroberts36 commented Dec 19, 2023 •

edited

Loading

Yurlungur left a comment

lroberts36 commented Dec 19, 2023

Yurlungur commented Dec 19, 2023

adamdempsey90 left a comment

pgrete left a comment

Yurlungur left a comment

Yurlungur May 22, 2024

lroberts36 May 22, 2024

Yurlungur May 22, 2024

lroberts36 May 22, 2024

Yurlungur May 22, 2024

lroberts36 May 22, 2024

pgrete left a comment

pgrete Jun 13, 2024

lroberts36 Jun 13, 2024

pgrete Jun 13, 2024

lroberts36 Jun 13, 2024

pgrete Jun 13, 2024

lroberts36 Jun 13, 2024

pgrete Jun 13, 2024

BenWibking commented Jun 18, 2024

lroberts36 commented Jun 18, 2024

		auto start_send = tl.AddTask(none, parthenon::StartReceiveBoundaryBuffers, mc1);
		auto start_flxcor = tl.AddTask(none, parthenon::StartReceiveFluxCorrections, mc0);

[Breaking change] Add fine field option #991

[Breaking change] Add fine field option #991

Conversation

lroberts36 commented Dec 19, 2023 • edited Loading

PR Summary

PR Checklist

Yurlungur left a comment

Choose a reason for hiding this comment

lroberts36 commented Dec 19, 2023

Yurlungur commented Dec 19, 2023

adamdempsey90 left a comment

Choose a reason for hiding this comment

pgrete left a comment

Choose a reason for hiding this comment

Yurlungur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrete left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWibking commented Jun 18, 2024

lroberts36 commented Jun 18, 2024

lroberts36 commented Dec 19, 2023 •

edited

Loading