Skip to content

Ensure the substeps we pass to SplitExplicitFreeSurface are the actual used substeps #4348

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Apr 6, 2025

in main this is not the case because the number of substeps is adapted to the averaging function by assuming that we cut the interval 2Δt in a substep number of substeps.

This PR calculates the correct ending fractional time based on the averaging function. Typically this corresponds to the time at which the averaging function reaches zero typically between 1 and 2 (times the baroclinic timesteps).
Other kernels might have a more complicated ending time (for example a constant function never reaches zero) for which users have to specify it themselves. For this purpose, this PR adds an AveragingKernel that stores also the ending fractional time and can be passed to the SplitExplicitFreeSurface constructor.

Finally the weights calculation cuts last_fractional_time * Δt in a substep number of substeps.

@simone-silvestri simone-silvestri changed the title Make sure the substeps we pass to SplitExplicitFreeSurface are the actual substeps Ensure the substeps we pass to SplitExplicitFreeSurface are the actual used substeps Apr 6, 2025
@glwagner
Copy link
Member

glwagner commented Apr 6, 2025

I tried to read to the code to figure out what's going on but I didn't get it quickly.

Isn't another option to change the meaning of what the user passes to SplitExplicitFreeSurface? It's unfortunate that these changes make the source code more complicated. Ideally it would simplify the source code.

@glwagner
Copy link
Member

Can a fix be to take the user input for substeps and multiply it by 2 before just reusing the existing machinery?

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