dtamayo / reboundx

A library for adding additional forces to the REBOUND N-body integration package
GNU General Public License v3.0
80 stars 60 forks source link

Spin intro changes #94

Closed hannorein closed 1 year ago

hannorein commented 1 year ago

I've changed the variable names to better correspond to the param names. Please check that this makes sense before merging.

This is NOT urgent, but there is one thing I was also stumbling upon while going over things. The total energy is calculated this was:

sim.calculate_energy() + rebx.spin_potential(sf)

But the total angular momentum is calculated this way:

rebx.calculate_total_angular_momentum()

Is there a reason for so many inconsistencies? Specifically:

On the REBOUND side, I suggest adding a new function sim.energy() and adding a deprecation warning to sim.calculate_energy(). I can make that change.

Not sure what the best approach is to the REBOUNDx side. I suggest:

dtamayo commented 1 year ago

Thank you so much for catching the naming mismatches left in Spinsintro.ipynb. And I definitely agree with everything else. I sent a pull request with your suggestion for REBOUND (also calculate_angular_momentum -> angular_momentum), and I've taken all of your suggestions. I did combine spin_potential and spin_kinetic energy into one and I think more consistently named it rebx.tides_spin_energy(). Angular momentum is a bit annoying in that sim.angular_momentum() + rebx.spin_angular_momentum() doesn't work since each of them returns a list, but we can cast them to np.array, or maybe switch them to reb_vec3d if someone has a good answer on stackoverflow

hannorein commented 1 year ago

Great! Thanks!

I agree, it's a bit annoying that the addition of lists doesn't work. One workaround would be to have the Vec3D class not be a subclass of ctypes.Structure but our own. We could write our own setters/getters for x, y, z, and keep a reference to the actual ctypes.Structure in our class but hidden from the user. Clearly a hack to circumvent some weird behaviour in numpy. Not sure if it's worth it. Let's wait a little longer to see if there is a simpler solution.