Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tags for worldtube iterations #5847

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

nikwit
Copy link
Contributor

@nikwit nikwit commented Mar 13, 2024

Changes the self force options to an Auto class and introduces the tags MaxIterations and CurrentIteration which will be required for the iterative scheme of the worldtube.

depends on #5831

@nikwit nikwit added the dependent Needs a different PR to be merged in first label Mar 13, 2024
@nikwit nikwit force-pushed the iteration-tags branch 2 times, most recently from 0ac68f0 to 3c1b9b2 Compare March 15, 2024 22:01
@nikwit nikwit removed the dependent Needs a different PR to be merged in first label Mar 15, 2024
Comment on lines 264 to +260
struct Mass : db::SimpleTag {
using type = double;
using option_tags = tmpl::list<OptionTags::Mass>;
using type = std::optional<double>;
Copy link
Contributor

Choose a reason for hiding this comment

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

An idea to maybe avoid having to hold individual optionals in tags for self force things and check if they are available: You could just store the SelfForceOptions that were constructed from the input file in a tag. Then that tag can be used instead of ApplySelfForce (you just check the optional), and also wherever you need it's parameters. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed ApplySelfForce now in favor of just checking whether MaxIterations is greater than 0. I think I would prefer to hide the extraction of these quantities in the OptionParsing, rather than having to get them from the struct each time step.

Comment on lines 288 to +275
*/
struct ApplySelfForce : db::SimpleTag {
using type = bool;
using option_tags = tmpl::list<OptionTags::ApplySelfForce>;
struct MaxIterations : db::SimpleTag {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change, you now implicitly convert a size_t to a bool wherever you had a check for ApplySelfForce. Could you change those to explicitly check if the size_t > 0? Also, depending on your thoughts about my other comment, this may be moot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed ApplySelfForce in fixup

Comment on lines +16 to +17
*/
struct InitializeCurrentIteration : tt::ConformsTo<db::protocols::Mutator> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to add this to the executable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is added in #5855

@nikwit nikwit requested a review from knelli2 March 22, 2024 14:16
Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Looks good. You can squash

@knelli2
Copy link
Contributor

knelli2 commented Apr 15, 2024

@nikwit Could you give this a quick rebase just to run CI again? Otherwise it looks good and I'll merge it after CI finishes.

@knelli2 knelli2 merged commit 52f20d7 into sxs-collaboration:develop Apr 17, 2024
22 checks passed
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.

2 participants