Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

dgegen
Copy link
Contributor

@dgegen dgegen commented May 7, 2024

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

Changes Made

  • Extended the docstring.
  • Renamed the radius parameter to ror (Ratio of planet radii to star radius). Reasoning: Using the term radius 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 named radius to ensure formal consistency with the Keplerian orbit class (although it should be noted that the two differ in that the radius attribute of that class has units of length). To remove this confusion, radius has been replaced by the attribute ror, and a radius property returning ror has been added to ensure comparability with other functions, such as limb_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 :))

dgegen added 2 commits May 7, 2024 18:40
- 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
@soichiro-hattori
Copy link
Collaborator

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

I'm happy to merge! @lgrcia?

@lgrcia
Copy link
Collaborator

lgrcia commented May 28, 2024

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
Copy link
Contributor Author

dgegen commented May 28, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

dgegen commented Jun 7, 2024

All options we have considered so far have merits downsides:

  • Include both radius and central_radius: This promotes consistency, but introduces unnecessary complexity by having two attributes where one might suffice, thus, an unnecessary degree of freedom.
  • Use radius with contextual meaning: This approach simplifies the interface but reduces code clarity as the meaning of radius changes depending on the context, requiring more documentation.
  • Use radius_ratio instead of radius: This makes the argument more self-explanatory, but sacrifices interface consistency.
  • Allow both radius and radius_ratio for initialisation: This provides flexibility, but requires documentation to clarify that the two terms are synonyms and cannot be used at the same time.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants