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

2 Parameter Limb-Darkening #75

Open
jenniferyee opened this issue Feb 2, 2023 · 2 comments
Open

2 Parameter Limb-Darkening #75

jenniferyee opened this issue Feb 2, 2023 · 2 comments
Assignees

Comments

@jenniferyee
Copy link
Collaborator

I created a new use case for passing limb-darkening coefficients to models and proposed implementation of models.

The major issues are:

  1. Adding "methods" and "limb_darkening_coefficients" to mm.Model.init()
  2. Creating a new class TwoParamLimbDarkeningCoeffs that inherits from LimbDarkeningCoeffs
  3. Passing dictionary of coefficients directly to LimbDarkeningCoeffs in init()
  4. What to name the variables for the TwoParam case? Two parameter limb-darkening takes both Gamma and Lambda, but naming the variable "gammas_and_lambdas" seems too long, so I just stuck to "gammas" (it is also possible to implement both: one for full transparency and the other for convenience).

Next step: write unit tests, including making sure that both (Gamma, Lambda) and (c, d) implementations work the same.

@rpoleski:

  1. Do you have any comments on the use case?
  2. It would be better for you to write the unit tests if I am implementing the 2-parameter limb-darkening algorithm.
@rpoleski
Copy link
Owner

rpoleski commented Feb 2, 2023

Why do you want to move "methods" and "limb_darkening_coefficients" to mm.Model.__init__()?

  1. Creating a new class TwoParamLimbDarkeningCoeffs that inherits from LimbDarkeningCoeffs

I've learned about open-closed principle recently (from the book by an author that you suggested to me :). Now I would say that both these classes should inherit from new abstract class, e.g., LimbDarkening.

  1. Do you have any comments on the use case?

The plotting part has lots of copy-pasted code - I would prefer to avoid it.

  1. It would be better for you to write the unit tests if I am implementing the 2-parameter limb-darkening algorithm.

OK.

@rpoleski rpoleski self-assigned this Feb 2, 2023
@jenniferyee
Copy link
Collaborator Author

I updated example 25 to reduce copy-pasted code.

It also reminded me that I want to add "plot_properties" as a keyword for models, so I added that to the use case to show how it would work.

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

No branches or pull requests

2 participants