rpoleski / MulensModel

Microlensing Modelling package
https://rpoleski.github.io/MulensModel/
Other
57 stars 15 forks source link

2 Parameter Limb-Darkening #75

Open jenniferyee opened 1 year ago

jenniferyee commented 1 year ago

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 commented 1 year ago

Why do you want to move "methods" and "limb_darkeningcoefficients" 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.

jenniferyee commented 1 year ago

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.