exoplanet-dev / jaxoplanet

Astronomical time series analysis with JAX
https://jax.exoplanet.codes
MIT License
41 stars 12 forks source link

Improve interface of `TransitOrbit` #179

Closed dgegen closed 3 months ago

dgegen commented 7 months ago

This pull request aims make the interface of TransitOrbit less confusing and more user-friendly.

Changes Made

We could consider temporarily keeping the radius as an additional attribute to ensure backward comparability, but in the interest of clear unambiguous nomenclature, I think it is best not to do so.

As always, feedback is greatly appreciated :))

soichiro-hattori commented 6 months ago

This looks good to me @dgegen! Thank you for doing this!

I'm happy to merge! @lgrcia?

lgrcia commented 6 months ago

Sorry for the delay in answering that. Thanks for this suggestion @dgegen and the review @soichiro-hattori!

I like the consistency between interfaces and explicit naming (i.e. not acronyms). What about simply indicating that radius is in unit of stellar radius in the docstring? Wouldn't that do the trick? I guess the comment in the docs was because of missing docstring initially.

dgegen commented 6 months ago

All good :))

I agree that consistency between interfaces is nice. In this case, however, I am not sure if it is not more preferable to have clear and self-explanatory variable names. I think ror or radius_ratio would be more explicit, highlight the conceptual difference between the usage in this class and in the keplerian orbit class, where the radius field has a units field that is part of the implementation, and eliminate the need to consult the documentation.

On the other hand, if we were looking for maximum consistency, we would probably allow the user to change the value of central_radius, and make both radius and central_radius have units. Then again, this adds complexity that is not really helping anyone.

soichiro-hattori commented 6 months ago

I think I agree with both of you here! Consistency in the interface is nice but I also think it can be a bit confusing if radius means slightly different things between the two orbit objects.

I don't know if this makes is convoluted but how do you two feel about allowing for either radius and ror to be used (we'd check to ensure only one of them is passed)? We could default to using ror in the docs and in general but also maintain some interface consistency by allowing the user to also use radius? @dgegen, @lgrcia

dgegen commented 5 months ago

All options we have considered so far have merits downsides:

I happen to prefer readability to consistency in this case, as I have some difficulty imagining cases where the consistency of the interface between TransitOrbit and the Keppler orbit is of much practical benefit, while I see the tangible benefit of having a self-explanatory interface.

That being said, I see the merits and drawbacks of all the options discussed, and if you, as much more significant contributors to this project, prefer another option, I'm happy to adapt.

lgrcia commented 5 months ago

Sorry for the delay here. Back on jaxoplanet! Let's name it radius_ratio for now and we'll discuss more if needed when preparing a new release.

dgegen commented 3 months ago

No bother at all! I finally got around to renaming all relevant instances of radius to radius_ratio, so in principle this pull request can be merged/rebased now :))