Improvements on PR: "New effects to Reboundx - Type I migration and inner disk edge"

[acpetit]

Hey

Here is my suggestion for the changes we talked about in Kaltrina's PR (https://github.com/dtamayo/reboundx/pull/69). I modified both modify_orbits_direct and modify_orbits_forces so that if the effect has the fields dedge and hedge, tau_a is modified and the planet stops.

I put the inner_edge function in a separate file that kept most of the comments from Kaltrina and the prototype in rebxtools.h.

I couldn't check the C examples (had a weird compilation bug on my Mac, but I think it's not related to Rebound) but the notebooks works as intended.

However, I am unsure about the main documentation and how we want to advertise the new effects there (I couldn't check the Doxygen files either 😅). I don't think it's best to show the parameter directly in the doc for modifyorbits*.

Cheers, Antoine

[coveralls]

Coverage remained the same at 84.844% when pulling 1f9d94ad850cd5e3efb2f6409f51c6dd487e52c4 on acpetit:typeImig into 063c676d897cb3544b569fd1135244f350371abf on dtamayo:master.

[dtamayo]

Hi Antoine,

Thanks so much for all the work Kaltrina and you put into this, I think it looks great and people will find it really useful! I see what you mean about where to put things in the documentation...I think with the way the documentation structure is set up, it would be simplest to just put the parameters in the documentation for type_I_migration, modify_orbits_forces, and modify_orbits_direct. Is it OK with you if I change the effect name to just type_I_migration instead of modify_orbits_with_type_I_migration?

I'll work on the C examples and documentation and get this merged. Thank you!

[acpetit]

Thanks! The name change is absolutely fine by me. The C examples should work, I just haven't compiled them to check it.

[Kaltrinak]

Thank you! Wonderful :)

[dtamayo]

Hi Antoine and Kaltrina,

I've sent you a pull request to your TypeI branch (https://github.com/acpetit/reboundx/pull/1), with some updates to the documentation and C examples. I changed some minor things to be consistent with the other examples, but most importantly, I

• changed the names of the parameters. We've moved to prefixing all parameters for new effects with an acronym for the effect name, so we now have inner disk edge -> ide_position, ide_width. I still need to move all previous names over to this convention, but it's the only way I see of continuing to add to the code base without all sorts of clashes. One thing I changed was initial_surface_density -> surface_density_1, and same for scale height, since the surface density isn't changing, and it's defined as the value at 1 code unit. I found that more intuitive than 'initial', but happy to change it back or to something else if you prefer

• I modified some of the documentation at the top of each .c file, and I annotated both the ipynbs. Would you mind giving them a 5 min read to make sure I didn't inadvertently mess something up?

Once you're happy with it, you can merge it into your typeImig branch, and I can merge into master. One thing I wasn't sure about was where the tau_tilde and analytical estimate for the semi major axis came from in the type I notebook. It might be nice to add a reference to where that comes from (maybe it's from your new paper!)

Thank you so much for all your work on this, I think it will be really useful!

[acpetit]

Hey,

All your changes make perfect sense! I actually agree with the prefix as a long term need (even though the names become less obvious). I think you should keep the current names for the vanilla effects both for backward compatibility and because they are general. However, it might be a good idea to write about it in the "Add a new effect" part of the documentation so that people use the convention.

I added a markdown cell for the analytical evolution in our example case. It is a very straightforward computation, I don't think it needs more.

[dtamayo]

Thanks for the updates, and for the really useful feedback. That's a good point about the names--it's a bit weird that some parameters would have prefixes and some wouldn't, but I guess as long as it's easy to look in the documentation for which parameter names you need to set it's not a big deal.

[Kaltrinak]

Hi Dan,

Thank you for your feedback and contribution! The changes look good, I agree on the name changes and as long as all the parameters have an explanation of what they are in the documentation, which they do, it should work fine for other users. I am very happy to see the code on the main branch! Glad it worked out so well and that it will be useful.

[dtamayo]

Thank you so much for this great work Kaltrina! I think a lot of people will find it useful!