Version 3 - main thread

[rpoleski]

What is decided to be done for Version 3:

• [x] derivatives of magnification and complicated structure of PointLens - architecture changes are made by @jenniferyee in Version3_1 branch; #107 #108
• [x] lists used for multi-source models - @jenniferyee implements in Version3_1 branch; #117
• [x] saving plots from examples (07, 08, and 16 are the only ones missing)
• [x] search for all "deprecated" are remove it - @rapoliveira
• [x] rename Caustics -> CausticsBinary and CausticsWithShear -> CausticsBinaryWithShear (and files) so that they're consistent with CausticsPointWithShear

Added later (only high-level ones):

• [ ] shifting alpha by 180 deg to match Skowron+11 conventions - too be done only when we make significant changes to binary lens models, so that it's very unlikely that somebody will be able to run on V3 the code written for V2
• [ ] making long-running tests shorter
• [ ] increasing number of characters per line

Also check MM_v3.md file for less changes that were not accepted yet.

Not decided yet and important:

• astrometric microlensing - probably too complicated for V3, hence, will got to V4,
• more advanced fitting fluxes of fluxes, e.g., keeping source colors similar or the same for data from different telescopes (at least 2 bands for at least 2 telescopes).

[jenniferyee]

For shifting alpha by 180 deg, I think we should make this change for V3 because it's wrong and we should fix it. However, is it possible to set up the fix in a similar way to MAG_ZEROPOINT so the user can set it to the V2 convention if desired.

[jenniferyee]

@rpoleski

Why are there two ways to calculate point source, binary lens magnification? We have both WittMao95 and VBBL. In the new architecture, this would be two separation magnification classes. Except the user never accesses these. Anyway, seems very complicated, so there must be a reason...

[rpoleski]

Long time ago VBBL had a feature/bug: it checked if source-lens distance is larger than 10 and if so, then point lens approximation was used. I don't know if it's still true. At some point we decided to leave both versions since they were already coded.

[rapoliveira]

There is a FutureWarning in the function _set_coords() of MulensData class: "coords will be deprecated in future. There is no reason to tie this to a given dataset"

Should this variable be removed at this point? This is the only deprecated missing in MulensData now.

[rpoleski]

@jenniferyee You have 2 defintions of FitData.get_d_A_d_rho(). Issue was spotted by @rapoliveira

[jenniferyee]

@rpoleski Is there an example for the exception that is raised from VBBL PSBL calculation (or even a unit test)? Just having the code check for an exception makes debugging hard. I introduced some bugs with my refactor, and they weren't caught because the calculation just switched to WM95. Relatedly, if VBBL returns a magnification vector, why do we have the check magnification < 1?

see BinaryLens.point_source_magnification() method.

[jenniferyee]

@rpoleski I don't think a try/except is the right way to handle VBBL PSBL failures.

1. The except clause is too broad, so we can miss fixable errors. In my tests, VBBL PSBL is failing because it is receiving the wrong number of arguments ("this function takes at least 7 arguments (4 given)"). This is also a problem in the master branch. So either this never worked or something changed with VBBL.

2. It would be better to model this after the way we deal with problems with point_source method --> point_source_point_lens method, so make it the responsibility of the user to change the calculation if it fails for mysterious reasons. Then, we would have "point_source" and "point_source_VBBL" --> VBBL PSBL calculation and "point_source_WittMao95" --> WM95 calculation. This can be easily added to the current architecture. It would also prevent the complete suppression of errors.

(I also need help fixing the VBBL PSBL error.)

[jenniferyee]

Note on "use_planet_frame": these should just be implemented as child classes of the standard frame.

So there would be BinaryLensPointSourceWM95Magnification() AND BinaryLensPointSourceWM95PlanetFrameMagnification().

These names are getting long, so we might also consider abbreviations, where BLPS_WM95Magnification inherits BinaryLensPointSourceWM95Magnification.

[rpoleski]

1. The except clause is too broad, so we can miss fixable errors. In my tests, VBBL PSBL is failing because it is
   receiving the wrong number of arguments ("this function takes at least 7 arguments (4 given)"). This is also a problem in the master branch. So either this never worked or something changed with VBBL.

I've checked that. It turned out that the bug was introduced while merging shear & convergence branch. I've made a quick fix in 3e31b57 . Proper solution requires more work on importing VBBL :(

[rpoleski]

First, I think it would be best to fully solve issue with VBBL 4 vs. 7 parameters in master branch, then merge it to Version3_1 branch.

Second, maybe this error was the caused somebody (Keto Zhang?) to report that our approach to importing C++ is too slow and we should try to pass vectors once instead of passing floats in a loop. Based on that comment, I've started vbbl_vectors branch, which is still not finished.

[jenniferyee]

Finished updating documentation. Still need help with fixing binarylens calculations. This is a prerequisite for finishing the binarylenswithshear refactor.

Next task: fixing binary source implementation. On deck: replacing the set_magnification_objects with "factories" to make it easier for the user to add their own magnification calculations. Side quest: create a parent MagnificationObject class of which the children will be PointSourcePointLensMagnification and BinaryLensPointSourceMagnification, see comments in binarylens.py

[jenniferyee]

@rpoleski documentation for BinaryLensPointSourceWM95Magnification has missing information. See boldface.

[rpoleski]

@jenniferyee Do you mean the reference to WM95 paper? If not, then please be specific.

[jenniferyee]

Status Update: Implemented a SourceParameters class and the ModelParams unit tests now pass. However, Example 11 and several other binary source unit tests don't work because it checks for a "ModelParameters" object rather than a "SourceParameters" object. In addition, there is a bunch of code in "SourceParameters" that is copy-pasted directly from ModelParameters. This means these two objects are related. The question is whether they are both children of a "MulensParameters" class or whether "ModelParameters" is a child of "SourceParameters." (I don't think SourceParameters is a child of ModelParameters because SourceParameters is more simple.) Regardless, the checks for "ModelParameters" objects need to be updated.

Relatedly: there is no real reason to store the source information differently for 1 source rather than multiple sources.

[rpoleski]

I was supposed to make notes on classes called factories (wiki link). Here is an example code:

model_1 = mm.GetModel({'t_0': 0, 'u_0': 0.5, 't_E': 10})
print(type(model_1))
# returns ModelPointSourcePointLens

model_2 = mm.GetModel({'t_0': 0, 'u_0': 0.5, 't_E': 10, 's': 1.5, 'q': 0.1, 'alpha': 123.})
print(type(model_2))
# returns ModelPointSourceBinaryLens

Note - a single class GetModel produced objects of different types based on the input it got.

[jenniferyee]

Current thoughts on refactoring ModelParameters to support multiple sources: Because we can add various components to a model independently (e.g. additional sources or lenses, parallax), this is similar to how astropy.modeling allows you to add various models together. Ergo, the ModelParameters refactor should/could follow the basic architecture of astropy.modeling.CompoundModel. (still figuring out how that would work in practice) https://docs.astropy.org/en/stable/api/astropy.modeling.CompoundModel.html#astropy.modeling.CompoundModel

https://docs.astropy.org/en/stable/_modules/astropy/modeling/core.html#CompoundModel

For more on simplifying architectures using inheritance, see also: https://realpython.com/inheritance-composition-python/

[jenniferyee]

Spent some time thinking about class diagrams, inheritance, and the order of adding effects. Although we might think finite source effects, extra lenses, parallax, etc. can be added in any order, the code needs a specific structure for adding these components. I propose:

1. Basic Model
2. N x Extra Lenses (Including Orbital Motion)
3. Finite (Primary Source)
4. M x Extra Sources (Including Finite Source, then xallarap)
5. Parallax

Hence, a Binary Lens model with Binary Sources (both exhibiting finite source effects) including parallax would be built as follows:

Basic Model (t0, u0, tE)

• BinaryLens Model (s, q, alpha)
• Finite Source (rho_1)
• Second Source (t0_2, u0_2)
• Second Source Finite Source (rho_2)
• Parallax (piEN, piEE)

A PointSource model with a Binary Lens exhibiting full keplerian orbital motion and parallax would be:

Basic Model (t0, u0, tE)

• BinaryLens Model (s, q, alpha)
• Keplerian motion (ds/dt, dalpha/dt, dz/dt)
• Parallax (piEN, piEE)

A triple source, point lens model, with finite source effects only for the third source: Basic Model (t0, u0, tE)

• Second Source (t0_2, u0_2)
• Third Source (t0_3, u0_3)
• Third Source Finite Source (rho_3)

[rpoleski]

I'm still not getting why you want to change ModelParameters. For me the current master branch is quite good. Ok, one obvious change is to keep multiple sources as a list, not hard-coded _1 and _2.

Minor comment:

A triple source, point lens model, with finite source effects only for the third source: Basic Model (t0, u0, tE) ...

I will keep our old decision to call these parameters (t_0_1, u_0_1, t_E) if there is more than one source. The idea was that parameters ending in _N are for specific source and all other (most importantly t_E but also pi_E_E/N) are shared by all sources. This approach seems clear for user and relatively easy to code.