astrostat / pylira

A Python package for Bayesian low-counts image reconstruction and analysis
9 stars 7 forks source link

How much, if any, Sherpa code could pylira rely on? #48

Open DougBurke opened 2 years ago

DougBurke commented 2 years ago

pylira could make sherpa either a required or optional dependency. Would this actually help (e.g. Sherpa does have convolution code - e.g #47 - but I haven't timed it to compare against scipy or the code in #47)?

I was actually thinking about this from today's meeting where we went through https://pylira.readthedocs.io/en/latest/pylira/user/tutorials/notebooks/point_source.html - for instance, Sherpa has a bunch of model classes that could be used to complement pylira.data - and it comes with the cash statistic - as used in https://pylira.readthedocs.io/en/latest/pylira/user/tutorials/notebooks/mcmc-deconvolution-intro.html - but probably harder to recognize rather than implementing it directly.

anetasie commented 2 years ago

@DougBurke interesting idea to consider sherpa as part of the dependency. We typically used sherpa to generate the baseline model images, so part of the full analysis includes already a sherpa step before running lira.

adonath commented 2 years ago

I think in general one could be rather liberal in adding dependencies, where it helps and does not make the installation much more complex. Probably meaning that pip install some_package must work on the most common architectures. I was thinking in a similar way of possibly adding gammapy.maps for the data structures, but so far the need was small. I think the default should be (possibly for using models as well) to rely on astropy whenever possible.

In the specific case of #47 I found the astropy.convolution.convolve_fft introduced too much overhead and resulted in the example running a factor of ~5-10 longer compared to the standard scipy.signal.fftconvolve. However the pip install scipy for the M1 architecture is a bit more complex as it requires installing OPENBLAS first independently (see e.g. https://github.com/scipy/scipy/issues/13409) and the setup with e.g. tox is not straightforward (there are wheels now, but they require MacOS >12.0). So as a last option I decided to include the minimal pure Numpy implementation, but this should not never be the first choice. And I would consider it a preliminary solution until the scipy installation on M1 has improved.

hamogu commented 2 years ago

@adonath Since you have a solution, this does not matter much, but just to note that there has been very recent activity in astropy to speed things up to get closer to numpy/scipy: https://github.com/astropy/astropy/issues/11242