Skip to content

Conversation

@Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Jun 27, 2022

PR Summary

Given the segfaults happening due to PostInitializationCommunication, I'm adding a runtime option to disable it. Adds an input parameter in the phoebus block.

In the long term, we need to pull #118 through. Alternatively, changes upstream in Parthenon may make this driver stage unnecessary. @brryan what are your thoughts?

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by calling scripts/bash/format.sh.
  • Explain what you did.

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok to me as a temporary fix since (I think) the PostInitializationCommunication is only necessary for periodic boundary conditions.

Comment on lines +36 to +37
params.Add("do_post_init_comms",
pin->GetOrAddBoolean("phoebus", "do_post_init_comms", false));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this default to off or on? I would say default to on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think default off is OK since it is currently broken -- can switch to on when it gets fixed

Copy link
Collaborator

@brryan brryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +36 to +37
params.Add("do_post_init_comms",
pin->GetOrAddBoolean("phoebus", "do_post_init_comms", false));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think default off is OK since it is currently broken -- can switch to on when it gets fixed

@jti-lanl
Copy link
Collaborator

This runs successfully on 4 devkit nodes.
I can't comment on the accuracy of the code with/without the boundary inits, but it removes the impediment.

@jti-lanl jti-lanl closed this Jun 27, 2022
@jti-lanl
Copy link
Collaborator

Dang it. Sorry. Didn't mean to close.

@jti-lanl jti-lanl reopened this Jun 27, 2022
Copy link
Collaborator

@jti-lanl jti-lanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay by me.

@Yurlungur Yurlungur merged commit 79bf072 into main Jun 27, 2022
@Yurlungur Yurlungur deleted the jmm/short-circuit-post-init-comms branch June 27, 2022 22:48
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.

7 participants