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

Substantial Python loop overhead for VBBL #104

Open
kmzzhang opened this issue Oct 2, 2023 · 6 comments
Open

Substantial Python loop overhead for VBBL #104

kmzzhang opened this issue Oct 2, 2023 · 6 comments

Comments

@kmzzhang
Copy link

kmzzhang commented Oct 2, 2023

Currently, the binary lens light curve is calculated in a python loop where each point is evaluated separately.

https://github.com/rpoleski/MulensModel/blob/master/source/MulensModel/magnificationcurve.py#L421

When using VBBL as the finite source method, the python loop overhead can slow things down by up to ~7 times compared to VBBL's own python wrapper, where the loop occurs in C++. This most apparent when VBBL is used for the full light curve (which itself decides automatically whether to trigger full FS calculation). Perhaps could aggregate points that use VBBL and move the loop into C++? I considered making a pull request but I realize this may involve refracting larger parts of the code.

@jenniferyee
Copy link
Collaborator

@kmzzhang We are about to do a major refactor for Version 3, so we can add this to the list. In particular, we are talking about having subclasses for models, which could include creating MagnificationCurve subclasses such as MagnificationCurveVBBL()

@rpoleski
Copy link
Owner

rpoleski commented Oct 3, 2023

We don't have to wait till v3. This seems relatively easy thing. I'm guessing we did it on the epoch-by-epoch basis because it's easier to pass a specific number of floats between python on C++, rather than arrays of unknown sizes.
Thanks @kmzzhang for bringing that up!

@kmzzhang
Copy link
Author

kmzzhang commented Oct 3, 2023

You're welcome. I believe this also applies to 2L1S point source when the SG12 solver in VBBL is used.

Since the finite source method is specified in time intervals in MulensModel, perhaps one way to refractor the code is to aggregate epochs by those intervals and calculate the magnifications?

@rpoleski
Copy link
Owner

rpoleski commented Oct 4, 2023

Yes, that's what I plan to do.

@rpoleski
Copy link
Owner

@kmzzhang Can you please share how you pass pointers/vectors of floats between python and C++? I see that there are different approaches to PyArg_Parse*() functions and would prefer not to re-invent the wheel, if you have already done so.

@kmzzhang
Copy link
Author

@rpoleski Apologies for the late reply. I don't have a particular way of doing this, but perhaps you could use Valario's way of making python wrappers: https://github.com/valboz/VBBinaryLensing/tree/master/VBBinaryLensing/lib. He has a python wrapper for the BinaryLightCurve function that takes array of times (ts). One could easily modify this function (and its wrapper) to take arrays of source locations y1s and y2s instead of times. Everything else would be the same.

@rpoleski rpoleski removed the version3 label Dec 30, 2023
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

3 participants