search

FastNFT / FNFT

Fast numerical computation of (inverse) nonlinear Fourier transforms
https://fastnft.github.io/FNFT/
GNU General Public License v2.0
42 stars 12 forks source link

Generalise preprocessing #61

Closed PJ-Prins closed 3 years ago

wahls commented 3 years ago

Dear all,

could you please have a look at the recent commits? I a few more things I stumbled upon:

ShrinivasJC commented 3 years ago
  • nse_fscatter and kdv_scatter now do very little but add a layer of abstraction. Should we remove these functions and call akns_fscatter directly?

I am not sure but I think our initial philosophy was that private functions with a specific prefix should only call functions with same prefix.

  • Could the weights for CF6_4 in akns_discretization_method_weights be written in a way that is precise if one changes the precision of the COMPLEX type (as it was done e.g. for CF5_3)?

Calculation of the weights of CF6_4 requires solving a system of nonlinear equations. Implementing that will be very involved without much gain at this stage.

ShrinivasJC commented 3 years ago

@wahls It shows that there is some conflict in the file src/fnft_nsev.c but for some reason I cannot see it. Can you please check it?

wahls commented 3 years ago
  • nse_fscatter and kdv_scatter now do very little but add a layer of abstraction. Should we remove these functions and call akns_fscatter directly? I am not sure but I think our initial philosophy was that private functions with a specific prefix should only call functions with same prefix.

The misc_ and poly_ functions are frequently called from private functions with other prefixes. From that perspective it would be fine for me to also call akns_ functions directly where appropriate.

  • Could the weights for CF6_4 in akns_discretization_method_weights be written in a way that is precise if one changes the precision of the COMPLEX type (as it was done e.g. for CF5_3)? Calculation of the weights of CF6_4 requires solving a system of nonlinear equations. Implementing that will be very involved without much gain at this stage.

Any chance there is some Matlab source that we could put here in a comment in case they have to be re-computed?

wahls commented 3 years ago

@wahls It shows that there is some conflict in the file src/fnft_nsev.c but for some reason I cannot see it. Can you please check it?

It should be fine now.

ShrinivasJC commented 3 years ago

@wahls It shows that there is some conflict in the file src/fnft_nsev.c but for some reason I cannot see it. Can you please check it?

It should be fine now.

Yes it is fixed now. Thank you.

PJ-Prins commented 3 years ago
  • nse_fscatter and kdv_scatter now do very little but add a layer of abstraction. Should we remove these functions and call akns_fscatter directly?

At least kdv_scatter should stay, to leave room for discretizations that are not in AKNS-basis. (In principle the preprocessing of q is not specific to AKNS either, but the preprocessing of r is, and currently there is no better place to put it.) I (still) think that the only way that the AKNS structure would be really advantageous when adding other PDE's (i.e. without code copying with some find and replace) is if nsev.c would currently merely be a wrapper of an aknsv.c with some NSE-specific function pointers. From aknsv.c you could call akns_fscatter and such.

  • The years in the headers of some of the changed files have not been updated.

Sorry, I will update it. Too bad github doesn't do that for us.

ShrinivasJC commented 3 years ago
  • nse_fscatter and kdv_scatter now do very little but add a layer of abstraction. Should we remove these functions and call akns_fscatter directly? I am not sure but I think our initial philosophy was that private functions with a specific prefix should only call functions with same prefix.

The misc_ and poly_ functions are frequently called from private functions with other prefixes. From that perspective it would be fine for me to also call akns_ functions directly where appropriate.

It is possible then. Just need to call the akns function to obtain the right akns discretization before calling akns_scatter.

  • Could the weights for CF6_4 in akns_discretization_method_weights be written in a way that is precise if one changes the precision of the COMPLEX type (as it was done e.g. for CF5_3)? Calculation of the weights of CF6_4 requires solving a system of nonlinear equations. Implementing that will be very involved without much gain at this stage.

Any chance there is some Matlab source that we could put here in a comment in case they have to be re-computed?

I took the coefficients from the referenced paper. The authors haven't shared any code to compute them.

wahls commented 3 years ago

At least kdv_scatter should stay, to leave room for discretizations that are not in AKNS-basis.

Ok, then let's keep them for now.

I (still) think that the only way that the AKNS structure would be really advantageous when adding other PDE's (i.e. without code copying with some find and replace) is if nsev.c would currently merely be a wrapper of an aknsv.c with some NSE-specific function pointers. From aknsv.c you could call akns_fscatter and such.

The usefulness of this approach might also depend on how similar the structure of the spectra for other PDEs are. I'd propose to look into this when another PDE that fits AKNS is to be added.

  • The years in the headers of some of the changed files have not been updated. Sorry, I will update it. Too bad github doesn't do that for us.

Thanks!

wahls commented 3 years ago

It is possible then. Just need to call the akns function to obtain the right akns discretization before calling akns_scatter.

Let's keep it as it is for now (see other reply).

Any chance there is some Matlab source that we could put here in a comment in case they have to be re-computed?

I took the coefficients from the referenced paper. The authors haven't shared any code to compute them.

Ok, bummer.

ShrinivasJC commented 3 years ago

Looks good to me now. Unless there are other comments, I propose to merge.

No more comments from me.

PJ-Prins commented 3 years ago

Looks good to me now. Unless there are other comments, I propose to merge.

No more comments from me.

Likewise.

wahls commented 3 years ago

Thanks, everyone!