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 LTS CMake Option in GenHarmBase Executables #6057

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

AlexCarpenter46
Copy link
Contributor

Proposed changes

Adds an option to either build in LTS or GTS for the executables that use GeneralizedHarmonicBase

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@AlexCarpenter46
Copy link
Contributor Author

If we don't use or build EvolveGhCce or EvolveGhCcm can I delete them in this PR (saw a comment on this in slack)?

@geoffrey4444
Copy link
Contributor

Do you understand why the tests fail? Let's discuss if needed!

@AlexCarpenter46 AlexCarpenter46 force-pushed the GHBase_LTS branch 5 times, most recently from 7de9fcc to db54fd0 Compare June 5, 2024 19:45
@AlexCarpenter46
Copy link
Contributor Author

Okay, got all the tests timing out figured out and clang-tidy so I think this is ready for a review :)

knelli2
knelli2 previously approved these changes Jun 6, 2024
Comment on lines 6 to 7
Check: parse;execute_check_output
Timeout: 8
Timeout: 30
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need this long of a timeout now that you decreased the slab size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah no, after looking at how long it took the tests in github, it seems 15 seconds should be good

@AlexCarpenter46
Copy link
Contributor Author

Squashed the timout change in :) @knelli2

geoffrey4444
geoffrey4444 previously approved these changes Jun 7, 2024
Comment on lines 31 to 34
# This is the smallest interval we'd need to observe time step/constraints. If
# you need it smaller you can edit it, but make sure to change the slab
# intervals in the EventsAndTriggers
InitialSlabSize: 0.001
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't restrict the slab size to line up with observations, particularly not to something this small. If you need more frequent output use dense output instead of forcing extra RHS evaluations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was sort of arbitrarily chosen to make the test not take forever. This wouldn't be very realistic for an actual evolution. Maybe I should change the comment to make it clear that you should change the slab size if you're using this yaml or use dense output if you want more frequent output.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, having the value small for the test is fine, but the comment shouldn't recommend doing observations that way.

# This is the smallest interval we'd need to observe time step/constraints. If
# you need it smaller you can edit it, but make sure to change the slab
# intervals in the EventsAndTriggers
InitialSlabSize: 0.5
Copy link
Member

Choose a reason for hiding this comment

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

Most of the observations in this file are based on the slab size, so you've decreased the amount of output by a factor of 50 here. For the constraint norms that might be fine, but it's probably not what you want for the actual data observations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was taken to match the inspiral.yaml and I assumed we'd want a similar amount of output during the ringdown.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. The dynamical timescales are shorter, so maybe people want more data? @geoffrey4444 or someone who actually analyzes runs want to weigh in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is what I actually did in the ringdown that worked; I'd suggest something similar in the pipeline:

Evolution:
  InitialTime: 0.0
  InitialTimeStep: 0.0002
  InitialSlabSize: 0.1
  StepChoosers:
    - Increase:
        Factor: 2
    - ErrorControl:
        AbsoluteTolerance: 1e-10
        RelativeTolerance: 1e-8
        MaxFactor: 2
        MinFactor: 0.25
        SafetyFactor: 0.95
  TimeStepper:
    AdamsBashforth:
      Order: 4

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any actual successful ringdown runs with global time stepping, so I don't have an opinion on good choices for that case

Copy link
Member

Choose a reason for hiding this comment

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

How often do you do observations? That's more what I'm worried about here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we know exactly how often we want to do observations. 0.1 for a slab size seems reasonable for now. We'll probably have to revisit this later though as we do more testing and actually do a successful ringdown with the pipeline. I think the more important part of this PR is the switch to LTS itself.

@wthrowe
Copy link
Member

wthrowe commented Jun 11, 2024

I'm fine with the settings in the files, but I still don't want comments telling people to shrink the slab to observe more.

@knelli2
Copy link
Contributor

knelli2 commented Jun 11, 2024

@wthrowe So what is your suggestion if more frequent observation is needed? Dense triggers?

@wthrowe
Copy link
Member

wthrowe commented Jun 11, 2024

Yes.

@AlexCarpenter46
Copy link
Contributor Author

Okay, changed the comments in the yamls to suggest dense output, let me know if this works @wthrowe

@wthrowe
Copy link
Member

wthrowe commented Jun 11, 2024

Yes, that's fine.

@knelli2
Copy link
Contributor

knelli2 commented Jun 11, 2024

Ignoring unrelated test timeouts/failures

@knelli2 knelli2 merged commit 3c2e9d3 into sxs-collaboration:develop Jun 11, 2024
20 of 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.

4 participants