rpoleski / MulensModel

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

Added __repr__ to event, model, mulensdata, and limb-darkening coeffs… #68

Closed jenniferyee closed 1 year ago

jenniferyee commented 1 year ago

… to give info I usually want + an example showing how they work.

jenniferyee commented 1 year ago

Note: I haven't updated the version number yet.

rpoleski commented 1 year ago

Let's follow scout rule - leave the code better than you found it. In this case the unit tests are missing. Also it would be good to add notes in class docstrings.

rpoleski commented 1 year ago

We still need unit tests for model.py (lines 118 and 122), event.py, and mulensdata.py.

rpoleski commented 1 year ago

One more aspect - how we want to note these in documentation? One approach is to add a sentence about it in __init__() docstrings for each class. Other possibilities?

jenniferyee commented 1 year ago

Extended example 24 to cover the new repr for coordinates, limb-darkening, and errorbar rescaling. Updated the formatting in MulensData.repr so the numbers line up. Looks ready to merge to me.

rpoleski commented 1 year ago
  1. Do you want full path to the ephemerides file to be printed? We don't print full path to the data file.
  2. I've made changes in MulensData.repr - see last commit above.
  3. Do we want to indicate which dataset is reference when printing Event? If "yes", then do we want it always, or only if it's not the first dataset? How to indicate it?

@jenniferyee - 2 new notes and 1 one old issue.

jenniferyee commented 1 year ago

test_Event.test_repr_full() / Event._repr_()

event = mm.Event(model=model, datasets=[dataset, dataset])

This produces weird behavior because Event.data_ref is sometimes stored as a MulensData instance. So, both datasets get flagged as being "data_ref".

I am going to change the test to use two different datasets. But I am leaving this comment here as a record of potentially weird behavior when using duplicate datasets within the same Event.

rpoleski commented 1 year ago
  1. Do you want full path to the ephemerides file to be printed? We don't print full path to the data file.

We decided to not change it and print the path as provided.

rpoleski commented 1 year ago

The last commit solves the issue with duplicated datasets (see last comment by @jenniferyee). Note that I removed one test that used duplicated datasets and changed a few other ones.

I don't see anything else to do here. This branch can be merged (don't forget to run sphinx and release new version).