Linear Perturbations update, Quality of Life and bug fixes

[DanieleFasano]

This update is divided in 2 sections:

1. The grid extension and the mcmc updates are related to what I was trying to do on the other project. I've seen that you were working on the planet rotation, so some of this may be redundant or even conflicting. I've left them separate so that you can easily choose not to add them to the master version

2. The Linear Perturbations update contains all the improvements I have performed on the reading and interpolation aspects of the linear perturbations, together with other improvements/bug fixes on the initial condition extraction and the burgers solution. I've seen that you changed something related to the interpolations, so we should be careful with this part

Here is a summary of the main changes:

• Linear perturbations reading
  • Added option to choose which linear perturbation to use
  • Added dictionary to keep track of the linear perturbation type and grid (to be generalised)
  • If Cartesian grid, interpolate on annulus
  • Simplified interpolation notation
  • If Cylindrical grid, mask perturbations and grid
• Interpolation
  • Simplified interpolation syntax
  • No cutting needed
  • Interpolation on an annulus segment with customisable dimensions
• Major changes to initial condition extraction on annulus method
  • Using proper Chi transformation for the initial profile
  • Added planet mass dependence to t coordinate
  • Using vector Eta transform to speed up the process
  • Sorting Eta (and Chi) array to solve discontinuity due to 2\pi modulus
  • Generalised eta_tilde definition (and corrected in inner disc) to get rid of obnoxious signs
  • Simplified physical rescaling
  • Added a parameter limiting numerical solution to outer/"inner" edge of the disc (inner edge to be defined, currently r_p/5)
• Transformations
  • Added non linear correction to the wake (currently set to 0 as it does not work)
  • Updated arguments of every function calling the wake to account for non liner correction (useful for the future only)
  • Added planet mass dependence in t coordinate
  • Corrected N wave definition, consistent with general definition of eta_tilde
• Burgers Equation
  • Simplified physical rescaling (done once in non_linear_perts.py)
  • Eliminated eta extension of profile, initial profile taken on annulus now
  • Implemented cyclic boundary conditions
  • Changed limit for numerical computation, equal to the edge of the disc
  • Implemented automatic break condition when reaching asymptotic limit (wip, to be tested)
  • Added some nice debug plots

[TomHilder]

Hi @DanieleFasano, I have started having a look at these updates and you have done an awesome job here. Apologies again for taking so long. Since the updates are very extensive, I am hesitant to merge straight onto the master branch, since I think it is unlikely that after resolving conflicts there will not be any bugs. Based on this I propose the following:

Currently wakeflow is split across the master and developer branches. I will reconcile these two branches first such that the master branch is up to date with the developer branch. I will do this before managing your pull request, since these updates are smaller. This should not be too bad, although I do think I have one or two bugs on the dev branch to handle.

After this, I will get you to re-make your pull request onto the developer branch instead of the master. Both of these branches will be identical by then, and so this just allows me to do some testing on the developer branch after handling the code conflicts. Once we are both happy that wakeflow with all the updates is stable enough, I'll merge the developer onto the master branch once again, and make a new major release.

Let me know if you are happy with this plan!

[DanieleFasano]

Hi @TomHilder, this seems reasonable to me, let me know if I have to do anything else in the meantime.

[TomHilder]

Hi @DanieleFasano, I have finished reconciling the master branch with my most recent changes. I have also made a minor release as a stepping-stone to the overhaul you have done here.

Additionally, I have create a new branch feb2023-overhaul specifically for merging this PR that is identical to the master branch. This is so that we don't edit the master branch until we are sure of stability (and I am scared of resolving the conflicts wrong haha).

The next step is for me to review your changes and resolve the conflicts :)

I am hoping to have everything done by July so that we have one (stable) version of wakeflow for when I come to Nice!

[TomHilder]

Hi @DanieleFasano, thanks again so much for the contribution! The majority of your changes look very good, and I actually think after reviewing them that the conflicts aren't going to be too much of a hassle to handle.

I have left a fair few comments about small changes for you to please implement. Most of these are just conventional things, or even just questions I had!

One common theme is that you have left quite a few print statements and plots that we don't want popping up for users who are not interested in the inner workings of wakeflow and just want a model (aka the observers). I am completely happy for the lines to stay in the code, they just need to be commented out (or put inside an if False: loop to be easily re-enabled). Another possibility is to define a further developer parameter to enable/disable all debugging prints and plots but I am fairly against this option since usually you don't want all of them.

Please implement the outlined small changes, let me know if anything is unclear (feel free to just reply to individual comments, I have GitHub notifications on) or you have any further questions.

Cheers!

[TomHilder]

Hi @DanieleFasano, thanks again so much for the contribution! The majority of your changes look very good, and I actually think after reviewing them that the conflicts aren't going to be too much of a hassle to handle.

I have left a fair few comments about small changes for you to please implement. Most of these are just conventional things, or even just questions I had!

One common theme is that you have left quite a few print statements and plots that we don't want popping up for users who are not interested in the inner workings of wakeflow and just want a model (aka the observers). I am completely happy for the lines to stay in the code, they just need to be commented out (or put inside an if False: loop to be easily re-enabled). Another possibility is to define a further developer parameter to enable/disable all debugging prints and plots but I am fairly against this option since usually you don't want all of them.

Please implement the outlined small changes, let me know if anything is unclear (feel free to just reply to individual comments, I have GitHub notifications on) or you have any further questions.

Cheers!

[TomHilder]

Hi @DanieleFasano thanks for addressing the requested changes so quickly! Everything looks good. I am curious though, I think when you've most recently merged your dev branch onto master, you've also pull across some changes that weren't present when I did the first code review last week, for instance discminer_interface.py definitely wasn't present. EDIT: I just looked at the contents of the merge PR you did above to address the requested changes, and indeed there are a few older commits from March and prior:

I'm not sure if this was on purpose but in practice it does not matter much. It just means that on-top of the changes I already reviewed, and your fixes, there are some additional changes not originally in the PR (for instance including discminer_interface.py).

I'm happy to include the discminer_interface.py on the master branch and PyPI distribution though. I am wondering what the difference with wakefit_interface.py is in practice? They seem to be identical apart from the class being renamed. I would prefer not to have two copies of the same code.

After we decide what to do about that, I will do a final look over the changes, handle the conflicts and finalise the merge.

[TomHilder]

Looks great, thanks @DanieleFasano! I'll do the merge tomorrow :)

[TomHilder]

Thanks @DanieleFasano I have now merged the PR. The conflicts in grid.py were very confusing and will probably require testing to reconcile, so I have kept both lots of code and commented the old (ie. my versions) out and labelled it. It is basically the code I added to rotate the planet location, but you have also changed the way the linear solution is read in. I'll work on reconciling these directly on the branch and once everything is good I'll merge onto the master branch.