-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve interface of TransitOrbit
#179
base: main
Are you sure you want to change the base?
Conversation
- Documented the `TransitOrbit` class. - Renamed the radius parameter to ror (Ratio of planet radii to star radius). Using the term `radius` for what is in practice the unitless ratio of planetary radii to stellar radii, simplly to ensure formal consistency with the Keplerian orbits class as far as I can tell, is confusing nomenclature - even for developers (see e.g. the comment `#TODO: Is it actually the radius ratio?` in the [Transit Fitting tutorial](https://jax.exoplanet.codes/en/latest/tutorials/transit/))! To remove this confusion, `radius` has been replaced by `ror` in the user interface, and a `radius` property that returns `ror` has been added to ensure comparability with other functions, such as `limb_dark_light_curve`.
- Make unit names consistent with kepler orbit - Remove properties from docstring
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. |
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 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. |
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 I don't know if this makes is convoluted but how do you two feel about allowing for either |
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 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. |
This pull request aims make the interface of
TransitOrbit
less confusing and more user-friendly.Changes Made
radius
parameter toror
(Ratio of planet radii to star radius). Reasoning: Using the termradius
for what is in practice the unitless ratio of planetary radii to stellar radii is a rather confusing nomenclature - even for developers, as the comment#TODO: Is it actually the radius ratio?
in the Transit Fitting tutorial illustrates to the user seeking to unravel the usage of this class. As far as I can tell, this attribute was originally namedradius
to ensure formal consistency with the Keplerian orbit class (although it should be noted that the two differ in that theradius
attribute of that class has units of length). To remove this confusion,radius
has been replaced by the attributeror
, and aradius
property returningror
has been added to ensure comparability with other functions, such aslimb_dark_light_curve
, in the background.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 :))