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

A few tweaks needed in core/fitting.py #313

Open
micahfolsom opened this issue Jan 20, 2022 · 4 comments
Open

A few tweaks needed in core/fitting.py #313

micahfolsom opened this issue Jan 20, 2022 · 4 comments

Comments

@micahfolsom
Copy link
Collaborator

While implementing the new model, I noticed a few things:

  1. The num args of ConstantModel.guess() and LineModel.guess() are not used
  2. The divides in expgauss() (in the same file) could be multiplies for potentially significant performance improvements (I would want to demonstrate this to be sure, though)
  3. Almost none of these functions/models have docstrings

I'll probably address these myself when I've got time

@jvavrek
Copy link
Contributor

jvavrek commented Jan 20, 2022

I took a quick look at changing expgauss with some toy data and found no significant speedup:

>>> %timeit -n 1000 -r 7 fitter.fit()  # with original /2
# 8.48 ms ± 31.7 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
>>> %timeit -n 1000 -r 7 fitter.fit()  # with new *0.5
# 8.54 ms ± 94.3 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

@micahfolsom
Copy link
Collaborator Author

Great, thanks for checking!

@cosama
Copy link
Contributor

cosama commented Jan 21, 2022

I think in general using multiplies is a good choice, particular with 2 (0.5), but you probably need a lot of these cases for it to really make a significant difference. It also looks like numpy is probably not as sensitive to this (maybe they do some optimizations here):

import numpy as np

a = np.random.rand(int(1e7))

%timeit a / 2

19.4 ms ± 1.01 ms per loop (mean ± std. dev. of 7 runs, 100 loops each)

%timeit a * 0.5

18.9 ms ± 386 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Also googling this a bit, it looks like there might be also the issue with 0.5 not actually being exactly 0.5 in it's floating point representation, hence you introduce some additional floating point rounding errors. Not sure how that works with division by 2. It probably is all hardware dependent.

I would use 0.5, but I feel it is a bit nitpicking.

@jvavrek
Copy link
Contributor

jvavrek commented Jan 21, 2022

@cosama 0.5 should be exactly 0.5 in floating point

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

No branches or pull requests

3 participants