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

Dealias sol before computing nonlinear terms #246

Merged
merged 10 commits into from
Jun 4, 2021
Merged

Conversation

navidcy
Copy link
Member

@navidcy navidcy commented May 31, 2021

This PR ensures that before any calculation of quadratic nonlinear terms the state vector sol is properly dealiased.

Closes #247.

@navidcy navidcy requested a review from glwagner May 31, 2021 21:10
@navidcy navidcy changed the title [WIP] Properly dealias before computing nonlinear terms Properly dealias before computing nonlinear terms May 31, 2021
@glwagner
Copy link
Member

Are there any docstrings that are updated? This changes default behavior and makes it so that filters can't be used without setting dealias=0 when constructing the grid. I think some of the examples use filters so they might need to be updated as well?

@navidcy
Copy link
Member Author

navidcy commented May 31, 2021

Are there any docstrings that are updated? This changes default behavior and makes it so that filters can't be used without setting dealias=0 when constructing the grid. I think some of the examples use filters so they might need to be updated as well?

  • This changes the equations that the module solve for I would say.. not "changes the default behavior". It just computes the nonlinear terms without any aliasing from high, spurious wavenumbers. Of course one could argue that a user may wanna simulate a flow with aliasing. To do that they may will have to either rewrite calcN! or set dealias=0 when constructing the grid. I'm simply arguing here that we were computing nonlinear terms wrong before and now they are correct.
  • filters can be used even without setting dealias=0 when constructing the grid
  • I agree that examples should be enhanced to better demonstrate/explain aliasing. I was thinking that this should be more appropriate in the FourierFlows.jl/Docs; see Docs section on dealiasing/filtered time-steppers FourierFlows.jl#283. What do you think?

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

I think the exponential filters we have defined assume that the flow is not dealiased or won't work as expected with dealiasing. We could get rid of these?

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

Agree -- I think we should remove the FilteredTimeSteppers. Perhaps in a different PR? But I do wanna give the users an easy way to apply filters if they want to. See FourierFlows/FourierFlows.jl#100.

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

I still feel that there is a disconnect in communication.

I am trying to say that regardless of filtering, hyper viscosity, and what not, dealiasing should be -- in principle -- done. Not doing dealiasing brings errors from aliasing. Oftentimes we can get away with thous with filtering or viscosity. But even in cases where filtering/viscosity seems to work "OK", I can't see why doing dealiasing on top of everything else shouldn't be done. In fact, I'm trying to argue that this is the proper way.

If we forget about examples, code changes, and technicalities that should come along with such a change, do you agree with the above statement?

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

I believe that the papers which introduce filtering do not dealias.

I agree that aliasing errors are real, however. I wouldn't try to enter an argument about whether or not a numerical computation without dealiasing is "improper" or "incorrect".

Not dealiasing is essentially a performance optimization which seems to rely on an assumption that power in high wavenumbers is negligible due to explicit dissipation. It's a common approximation in many spectral calculations --- proper or not.

I'd be fine with making dealiasing default and even getting rid of all the examples that use filtering. It's a bit of work to get there and does reduce available features, so there's some cause to pause and consider.

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

If we forget about examples, code changes, and technicalities that should come along with such a change, do you agree with the above statement?

No, I don't agree that dealiasing "should" be done in all circumstances.

I think dealiasing is necessary to eliminate aliasing errors. Aliasing errors often compromise solutions with significant power at high wave numbers. This often occurs in marginally resolved simulations whose resolution is close to being as low as necessary to resolve the dynamics of a particular flow.

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

I believe that the papers which introduce filtering do not dealias.

I agree that aliasing errors are real, however. I wouldn't try to enter an argument about whether or not a numerical computation without dealiasing is "improper" or "incorrect".

Not dealiasing is essentially a performance optimization which seems to rely on an assumption that power in high wavenumbers is negligible due to explicit dissipation. It's a common approximation in many spectral calculations --- proper or not.

I'd be fine with making dealiasing default and even getting rid of all the examples that use filtering. It's a bit of work to get there and does reduce available features, so there's some cause to pause and consider.

Oh, I hope the disagreement was not due the use of "proper" in the PR title... I can happily take that back! It's not about being proper or not. My main point was that there always exists some aliasing error and if users decide to ignore it because it doesn't seem to be important in the particular calculation or because they want the code to run faster then fair enough -- they can do it.

@navidcy navidcy changed the title Properly dealias before computing nonlinear terms Dealias sol before computing nonlinear terms Jun 1, 2021
@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

If we forget about examples, code changes, and technicalities that should come along with such a change, do you agree with the above statement?

No, I don't agree that dealiasing "should" be done in all circumstances.

I think dealiasing is necessary to eliminate aliasing errors. Aliasing errors often compromise solutions with significant power at high wave numbers. This often occurs in marginally resolved simulations whose resolution is close to being as low as necessary to resolve the dynamics of a particular flow.

OK, I see this point. Now I'm skeptical about this whole PR...

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

It's probably a positive change. Users looking for a speed up can set dealias=0 and interpret their resolution as the full resolution. Most people perform computations at marginal resolution. Filtering seems like an alright alternative to dealiasing, but hard to interpret. The conservative approach is to dealias so its a good default.

I think dealiasing by default is clearer though using the 3/2 rule, because people won't get confused about the resolution their running at.

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

Yes, but that requires padding, etc... Also, I never understood how FFTs are optimised then because e.g. 128*3/2 = 192 and 192 is not a power of 2....

I know some argue for double padding, e.g. 128->256! But that seems a lot of waste, in particular when you think of 3D sols...

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

An alternative to this PR would be to demonstrate dealiasing in an example. So that could, e.g.,

dealias!(sol, grid)
stepforward!(prob)

But question that arises (in my head at least) is: Is this enough for multi-step time-stepping? E.g., RK4 involves 4 calls of calcN! but dealiasing only happens once.

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

These days FFTs are optimized for grids that are products of many possible small numbers like 2, 3, 5, 7, and possibly more.

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

An alternative to this PR would be to demonstrate dealiasing in an example. So that could, e.g.,

dealias!(sol, grid)
stepforward!(prob)

But question that arises (in my head at least) is: Is this enough for multi-step time-stepping? E.g., RK4 involves 4 calls of calcN! but dealiasing only happens once.

I don't think it's enough. To completely eliminate aliasing errors we have to dealias after every nonlinear product I think.

@navidcy
Copy link
Member Author

navidcy commented Jun 1, 2021

Then the example could demonstrate how the user can import calcN! and redefine it, e.g.

import GeophysicalFlows.TwoDNavierStokes.calcN!

function calcN!(N, sol, t, clock, vars, params, grid)
  dealias!(sol, grid)
  
  TwoDNavierStokes.calcN_advection!(N, sol, t, clock, vars, params, grid)

  TwoDNavierStokes.addforcing!(N, sol, t, clock, vars, params, grid)
      
  return nothing
end

Or this gets to much into the nitty-gritty of the code?

@glwagner
Copy link
Member

glwagner commented Jun 1, 2021

You could set the default aliased_fraction=0 and then illustrate the effect of aliased_fraction=1/3 and explain how this implies a reduced effective resolution? If the simulation isn't well resolved then the aliasing errors might be apparent, illustrating the value of dealiasing.

@navidcy
Copy link
Member Author

navidcy commented Jun 3, 2021

Let's first wait for FourierFlows/FourierFlows.jl#285 first.

@navidcy navidcy merged commit bf62e73 into master Jun 4, 2021
@navidcy navidcy deleted the ncc/proper-dealias branch June 4, 2021 06:29
@navidcy navidcy restored the ncc/proper-dealias branch June 4, 2021 06:31
@navidcy navidcy deleted the ncc/proper-dealias branch October 5, 2021 20:21
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.

Should we enforce dealiasing before any calculation of nonlinear terms?
2 participants