tomhilder / wakeflow

Generate and manipulate semi-analytic models of planet wakes

• 👋 Hi, I’m @TomHilder
• 👀 I’m interested in modelling how young planets interact with their host protoplanetary disks
• 🌱 I’m currently completing my PhD in astrophysics at Monash University
• 📫 How to reach me: [email protected]

wakeflow's Issues

[enhancement] Add stricter coding style convention and add contribution guidelines

A coding style-enforcement tool such as black could be adopted and integrated as a Github Action.

We should also have contribution guidelines.

[question] Grid vertical extent

Why is the maximum vertical extent of the grid fixed to one pressure scale height (at R_out)? To date the majority of velocity kinks reported in the literature have been identified in 12CO, which is normally between 2-4H over the disc midplane, and at large radial separations. I would then suggest you to make z_max an optional argument too with a default value of e.g. 3H at R_out.

openjournals/joss-reviews#4863

Can only make results with anti-clockwise rotation

Fixing the inner wake evolution broke the ability to have both clockwise and anti-clockwise rotating disks.

This should be fixed by rotating the results appropriately at the end, if the use has specified clockwise. It is too much of a pain to keep track of a minus sign throughout the whole calculation.

[enhancement] Add code of conduct

Add an appropriate code of conduct.

[enhancement] Finish porting `spiralmoments`

The part of spiralmoments that allows to read-in velocity field data and rotate/project it should also be ported. It should be easy for the user to generate a peak-velocity map prediction by having spiralmoments call wakeflow for them, or similar. This will likely require #26 to be completed first.

After this, the README should be updated to reflect the fact that spiralmoments has been ported completely. A Jupyter notebook with examples should also be added to the docs.

[enhancement] Add issues template

See title.

[enhancement] Improve user-interface for generating models

The interface should be changed so that it does not require writing and reading files at all. The output data of wakeflow is small enough to be sensibly kept in memory, for instance for use in a self-contained script, or an Jupyter notebook for analysis.

This is sort of already implemented in wakefit_interface.py, but it’s not done all that well and also is not accessible to the user easily right now.

This would be a relatively straight-forward change, but it should be left until after the new features from #21 have been merged.

[bug] `phi_planet` parameter updated broke cylindrical grid results

Adding the option to move the planet to different azimuths has broken the cylindrical grid results. When running with a cylindrical grid, the linear regime is empty. Some issue is probably occurring with interpolation/masking.

[bug] Automated tests

The automated tests fail if the standard pytest is used. Your tests require the pytest-cov plugin to support coverage reports. Please edit readme and documentation accordingly or modify functionality.

This is part of the reviewing process for JOSS openjournals/joss-reviews#4863

[enhancement] Planet location

Please consider having the position angle of the planet (PAp) as an optional argument in wakeflow too (i.e. not only for the interface with MCFOST). This will make it easier to compare output velocities to actual observations and will also facilitate the link between wakeflow and other codes in the community such as eddy or discminer.

openjournals/joss-reviews#4863

[enhancement] Improve warnings and exceptions system

wakeflow errors and warnings should be changed to adhere to standard Python conventions.

Currently, wakeflow uses print statements to warn users about certain parameter choices, and generic Exceptions for incompatible or otherwise non-sensical parameter choices. This does not really adhere to standard practice; warnings.warn should be used for the warnings, and more specific errors should be used instead of Exceptions. For example ValueError for a bad parameter choice. The current implementation is due to my limited knowledge of error handling at the time I wrote the code. Users should be able to catch certain warnings and errors and handle them appropriately if they desire, which is impossible in the current implementation.

[documentation] Various suggestions

• Missing density units in 'Wakeflow Parameters' section.
• In the tutorial consider using ax.contourf(X[:,i,:], Y[:,i,:], rho[:,i,:], levels=256) instead of ax.pcolormesh to avoid confusion with the order of x, y axes and the need to use the transpose of rho. Note that pcolormesh is intended for irregular rectangular grids and thus not really needed here.
• In the tutorial please consider adding a few words explaining why having a planet-to-thermal mass ratio > 1 may lead to unreliable results in the analytical framework.
• In the 'Disk Structure' section you should explicitly mention that the additional factor in the rotation velocity is due to the presence of radial pressure forces and provide actual link(s) to the reference(s) e.g. Nelson et al. 2013.
• A few more details should be added as to what the user is seeing as output. For instance, in the tutorial you show an example of how to plot the resulting density field at different scale heights which may give the wrong impression that the velocity field can also vary as function of height (which is not accounted by the analytical theory in which the code is based). In this case please remind the user that the output velocity field is computed for the disc midplane only.

openjournals/joss-reviews#4863

[enhancement] Sanity checking `H/r_p` for user-specified global linear perturbations

Quoted from PR #21 :

Tom: I feel there needs to be a further sanity-check/warning in the case that the user is providing either global or simulation perturbations. H/r_p needs to be identical in the provided data and as specified to wakeflow no? It would be good to really reinforce this to the user (obviously we can't actually check the H/r_p in the linear perturbations they provide).

Daniele: Actually we could do something about it, maybe we can work on this together when you are in nice. Andrés is parsing the log files in discminer, checking if they are compliant with the standard nomenclature. We could change the names of the perturbations accounting for the different parameters and parse the name of the files, checking if H/r_p is actually the same

[stability] Ensure stability of overhaul updates after merge

Ensure the stability of the feb2023-overhaul branch after merging #21, so that it can be itself merged onto the master branch.

[enhancement] Automate version control and PyPI publishing using actions

See title. It's slow having to manually run bumpver locally, build locally, upload to PyPI locally. And it is very easy for the version numbers between GH master branch and PyPI to de-sync.

[enhancement] Improve `mcfost` interface to allow users to edit .para file parameters

The interface to mcfost should be changed such that wakeflow only specifies the strictly required parameters from the .para config file for mcfost.

Currently, wakeflow tells mcfost the necessary grid and physics, but also guesses some other parameters that are probably sensible for the user but potentially not desired. It is also NOT documented what these are and why they are the specific values. This is definitely poor practice. This should be changed such that besides the required physics parameters, the other parameters are (somehow) set by the user. Probably just by having them provide a .para file, where the necessary parameters are overridden by wakeflow.

Additionally, the mcfost parameter file is currently distributed with wakeflow. This is bad practice. wakeflow should include a script to download the .para file using the user's mcfost installation.

Finally, mcfost should be called using the run function from pymcfost instead of using a sys call to mcfost. This is more effectively future-proof as pymcfost is more likely to be updated as required than wakeflow's interface. It would also allow the user to pass through additional keywords for calling mcfost as they are arguments in the pymcfost function.