-
Notifications
You must be signed in to change notification settings - Fork 2
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
New resolution matrix implementation #181
Conversation
which yields a nice speedup. Merge the needed functionality of the emline_fit sparse matrix, so that we don't have to pass two sepearate resolution matrices to different parts of the code.
…ices * don't allocate unused local reference to pixkms_bounds in ContinuumTools * emit a log message when we use an acceelerated FFT library * store frequently used, frozen lists in input data dictionary as fixed-sized tuples or arrays for efficiency
Thanks @jdbuhler. The fitting code looks good but running
Can you take a look and push a fix? Also, can you please update the |
* update documentation of sparse emline Jacobian to std
further limit ftol to 1e-3 for greater speed.
* attempt to clean up some formatting warnings * add new-style docstrings to emline_fit/utils.py
of functions inner where they have only one caller
* consistently import jit from numba instead of saying '@numba.jit'
@@ -592,7 +589,7 @@ def call_nnls(modelflux, flux, ivar, xparam=None, debug=False, | |||
|
|||
|
|||
@staticmethod | |||
@numba.jit(nopython=True, fastmath=True, nogil=True) | |||
@jit(nopython=True, nogil=True, fastmath=True) | |||
def attenuate(M, A, zfactors, wave, dustflux): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we want to change fastmath=False
here and elsewhere?
I don't think so. There was one place that it needed to be turned off, which we did a couple of weeks ago.
You can do it if you want to see if it makes a big difference.
Jeremy
On Sep 5, 2024 08:08, Moustakas ***@***.***> wrote:
@moustakas commented on this pull request.
________________________________
In py/fastspecfit/continuum.py<#181 (comment)>:
@@ -592,7 +589,7 @@ def call_nnls(modelflux, flux, ivar, xparam=None, debug=False,
@staticmethod
- @numba.jit(nopython=True, fastmath=True, nogil=True)
+ @jit(nopython=True, nogil=True, fastmath=True)
def attenuate(M, A, zfactors, wave, dustflux):
Didn't we want to change fastmath=False here and elsewhere?
—
Reply to this email directly, view it on GitHub<#181 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AH4ORJ4TIYP6CL43E7PP2HTZVBCRTAVCNFSM6AAAAABNQXCNSWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBSG4ZDENZVHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look fine, but either this PR or #180 has led to non-negligible changes in the continuum-fitting results (as you warned me, off-list).
I added just a couple comments. After you make those changes, let's go ahead and merge this PR and then I'm going to test the fitting results on a larger sample.
py/fastspecfit/igm.py
Outdated
@@ -11,7 +11,7 @@ | |||
|
|||
|
|||
class Inoue14(object): | |||
r""" | |||
""" | |||
IGM absorption from Inoue et al. (2014) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the "raw string" formatting, integration tests give a DeprecationWarning
. Can we please revert this change?
/global/u2/i/ioannis/code/desihub/fastspecfit/py/fastspecfit/igm.py:14: DeprecationWarning: invalid escape sequence '\m'
py/fastspecfit/continuum.py
Outdated
modelflux = np.empty(self.wavelen) | ||
|
||
for icam, (s, e) in enumerate(camerapix): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little (a lot?) OCD, but can we remove the added blank 8 spaces on lines 754 and 756? (My configuration of spacemacs shows these blank characters, so I've been cleaning them up.)
@@ -206,8 +206,9 @@ def init_ffts(self): | |||
if find_spec("mkl_fft") is not None: | |||
import mkl_fft._scipy_fft_backend as be | |||
sc_fft.set_global_backend(be) | |||
|
|||
|
|||
self.convolve = sc_sig.convolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove blank spaces.
self.convolve = sc_sig.convolve | ||
log.info('Using mkl_fft library for FFTs') | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a debugging comment?
Ok. I'll do this as soon as I am back in front of my computer.
The biggest output-altering change was changing ftol from 1e-5 to 1e-3.
Jeremy
On Sep 5, 2024 08:31, Moustakas ***@***.***> wrote:
@moustakas approved this pull request.
The code changes look fine, but either this PR or #180<#180> has led to non-negligible changes in the continuum-fitting results (as you warned me, off-list).
I added just a couple comments. After you make those changes, let's go ahead and merge this PR and then I'm going to test the fitting results on a larger sample.
________________________________
In py/fastspecfit/igm.py<#181 (comment)>:
@@ -11,7 +11,7 @@
class Inoue14(object):
- r"""
+ """
IGM absorption from Inoue et al. (2014)
Without the "raw string" formatting, integration tests give a DeprecationWarning. Can we please revert this change?
/global/u2/i/ioannis/code/desihub/fastspecfit/py/fastspecfit/igm.py:14: DeprecationWarning: invalid escape sequence '\m'
________________________________
In py/fastspecfit/continuum.py<#181 (comment)>:
modelflux = np.empty(self.wavelen)
-
+
for icam, (s, e) in enumerate(camerapix):
It's a little (a lot?) OCD, but can we remove the added blank 8 spaces on lines 754 and 756? (My configuration of spacemacs<https://www.spacemacs.org/> shows these blank characters, so I've been cleaning them up.)
________________________________
In py/fastspecfit/templates.py<#181 (comment)>:
@@ -206,8 +206,9 @@ def init_ffts(self):
if find_spec("mkl_fft") is not None:
import mkl_fft._scipy_fft_backend as be
sc_fft.set_global_backend(be)
-
+
self.convolve = sc_sig.convolve
Remove blank spaces.
________________________________
In py/fastspecfit/templates.py<#181 (comment)>:
self.convolve = sc_sig.convolve
+ log.info('Using mkl_fft library for FFTs')
else:
Maybe this should be a debugging comment?
—
Reply to this email directly, view it on GitHub<#181 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AH4ORJ4X7OXSGDZ7SJP63HTZVBFIHAVCNFSM6AAAAABNQXCNSWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBSG4ZTGOBXGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
* make log msg about using mkl_fft debug instead of info * minor whitespace cleanups
* remove shell commands from tops of files that do not contain main or test for __main__
Thanks, @jdbuhler, I'm going to go ahead and merge this and then run on a few 10^4 spectra. |
Reimplement enough of desispec.resolution.Resolution to perform matrix-vector operations, which is all we do with it, with a significant speedup via Numba compared to the scipy.sparse implementation.
Retire the old res_emline version of the matrix, which is slower for dot product than either DESI or our new implementation. Allow conversion on the fly from our new impl to the internal rep used by res_emline, which is used way down in the guts of emline fitting but not otherwise. Switching away from res_emline for dot products does result in a small change in the output of emline fitting, which propagates to masking.
In addition, make two small but meaningful changes to least_squares call in continuum fitting: switch from lsmr to exact method for TRF algorithm, which reduces least_squares overhead, and use a looser ftol of 1e-3 instead of 1e-5, which greatly reduces the number of lsq iterations. (These should only be accepted if the output still looks OK!)
In passing, implement several small code cleanups that do not impact the results at all, including more efficient storage of some fields in the data dictionary and avoiding a copy in the _stellar_objective().