search

keflavich / imf

Simple tools to work with the Initial Mass Function
MIT License
44 stars 17 forks source link

Speedup of chabrier.distr #27

Closed segasai closed 3 years ago

segasai commented 3 years ago

Hi,

It's a simple change avoiding constructing the distr each time in Chabrier.

keflavich commented 3 years ago

This is fine, but I suggest making mmin and mmax immutable properties so that you can't do something like:

cb = Chabrier(mmin=1,mmax=5)
cb.mmin=2

and get confused

keflavich commented 3 years ago

(also lognormal_center and lognormal_width)

segasai commented 3 years ago

Yes sorry I forgot about this. But TBH, do we actually want to support the changes of mmin, mmax etc ? It looks like it's a constant source of bugs, as it requires updating all the internal variables etc.

keflavich commented 3 years ago

Indeed, I'm suggesting we make the attributes immutable (read-only properties) to avoid these bugs.

I think this is going to break some of my code - maybe some of the existing tools within the package - but we should do it the more correct way, which is only setting mmin/mmax at init time.

keflavich commented 3 years ago

at present, this PR will break this code, which wasn't super cleverly written:

https://github.com/keflavich/imf/blob/master/imf/imf.py#L510-L516

the fix is to instantiate a new instance of the selected distribution if mmax, mmin are specified, rather than grab a "cached" version.

The reason for the code above was to reduce the overheads in creating a synthetic cluster. I'm not convinced the time savings were ever worth it.

segasai commented 3 years ago

My suggestion is to put a test first that exercises that functionality. I don't like to refactor stuff that doesn't have a test.

keflavich commented 3 years ago

I will do that - sorry, haven't had a chance. Bump this if I leave it too long

keflavich commented 3 years ago

I'm merging this to continue the refactor work locally