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 several common time dependent options #6113

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented Jun 25, 2024

Proposed changes

Second PR in the series to revamp the time dependent options of the Sphere and BCO domain creators.

This PR adds common options for the

  • Rotation map
  • Expansion map
  • Translation map

These common options were designed so that different function of time types (PiecewisePolynomial or SettleToConst) could be chosen at runtime.

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

Depends on and includes #6109

@knelli2 knelli2 added this to the BBH First Paper milestone Jun 25, 2024
@knelli2 knelli2 added dependent Needs a different PR to be merged in first labels Jun 26, 2024
"acceleration."};
};

struct InitialValuesOuterBoundary {
Copy link
Member

Choose a reason for hiding this comment

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

Question: why doesn't "FromVolumeFile" just copy the FoT altogether, instead of essentially recreating it from values and derivatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So originally I didn't do this because I was lazy and I just read in the last FoT in the volume file rather than finding the correct observation ID. However, now that I think about it, there's likely a real issue.

Imagine a scenario where I'm writing volume data at simulation time $t_0$. Whenever we write the volume data in human time, that's when we grab the FoTs, serialize them, and write them. There could be a case where, by the time the FoTs were written, a control system has updated the FoTs past $t_0$. This will then cause issues upon restart because the expiration times of the functions of time are all messed up and not what was expected. The control system (at the moment) expects to set all expiration times and to just have FoT values at $t_0$ exactly. So I think for the time being, I think this is how things need to work.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so the FoT in the volume data can be wrong? That sounds like a bug that we have to fix. It also affects interpolation of the volume data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No not wrong. If you write volume data at $t_0$, then the FoTs will be valid at $t_0$, which is needed for interpolation and other stuff. That's guaranteed. It's just that the FoTs could also be valid at times after $t_0$ as well. This is because of asynchronous nature of the updates of the FoTs.

Copy link
Member

Choose a reason for hiding this comment

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

We currently retrieve the FoTs from the DataBox when volume data is written by the observer nodegroup, which is is not guaranteed to be close to the time the time step was taken. Is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

What I understood from a meeting discussion :

Currently we write the serialized functions of time at the end of the observation chain, eg in ContributeVolumeDataToWriter right before we call volume_file.write_volume_data(). This means the functions of time can have data from later than necessary in them. That is, if we write at $t_0$ and the functions of time expire at $t'>t_0$, then when we actually do the write we could have function of time data for $t>t'$ written to disk. Most of the time this doesn't matter because we only want to evaluate the functions of time at $t_0$. However, in the case where we want to use this data as a "checkpoint" (e.g. starting at a different resolution), the future state of the functions of time is no longer correct. What we need to do is essentially "drop" this future state. This PR provides one implementation for dropping that future state.

Copy link
Member

@nilsvu nilsvu Sep 11, 2024

Choose a reason for hiding this comment

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

Ok that's fine and we can merge it. Would another possible implementation be to just copy the FoT object from the volume data and add a function that drops future data from it? If that has the same effect then it seems a lot more robust to me and avoids a lot of code in this PR (since it just uses the copy constructor). Again, if the decision is that this is more work than to maintain the new code in this PR then fine with me.

Copy link
Member

Choose a reason for hiding this comment

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

This feature was historically intentionally omitted from the FoT's because the situation described above is the only case where you should legitimately be dropping times. We figured it would be safer to not allow dropping them because doing so would almost always be a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Ok wouldn't a function like get_clone_until_time(t) be a good implementation of this?

Copy link
Member

Choose a reason for hiding this comment

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

That could work, but I'd have to think about it more carefully. It does get around the issue for why we originally didn't allow a drop() function.

src/Domain/Creators/TimeDependentOptions/ExpansionMap.hpp Outdated Show resolved Hide resolved
@knelli2 knelli2 removed the dependent Needs a different PR to be merged in first label Jul 8, 2024
@knelli2
Copy link
Contributor Author

knelli2 commented Aug 13, 2024

@nilsvu I think we'll want to get this series of PRs in so we have the flexibility of initializing the FoTs

Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

You can squash

};

struct DecayTimescale {
using type = Options::Auto<double, Options::AutoLabel::None>;
Copy link
Member

Choose a reason for hiding this comment

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

What should this option be when continuing an evolution from volume data? Generally, there are lots of choices to make for all these options, but when I continue an evolution from volume data I just want to specify "FromVolumeFile" for the whole thing, don't I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be None. Yeah the way it is now probably isn't ideal. Yes just specifying "FromVolumeFile" would be nice, except that'd basically mean moving the "FromVolumeFile" part up a level and adding a separate "ExpansionMap" class for it.

I could change this from None to Auto so it's at least the same as the other options so when you are specifying that you want to continue from volume data, you just say Auto for everything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nilsvu Went ahead and made this Auto and just added a note in the help string about it

"option."};
};

struct DecayTimescale {
Copy link
Member

Choose a reason for hiding this comment

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

Same for this one

@knelli2
Copy link
Contributor Author

knelli2 commented Sep 9, 2024

@nilsvu Rebased

@nilsdeppe nilsdeppe merged commit 32dd542 into sxs-collaboration:develop Sep 18, 2024
22 of 23 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.

3 participants