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

faster emission line sims for MKL>=2018.0.2 #390

Merged
merged 2 commits into from Jul 17, 2018
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jul 16, 2018

This PR fixes desihub/desitarget#323 where select_mock_targets became mysteriously slower for BGS and ELG using the latest desiconda. I traced this to a single line in desisim.templates.EMSpectrum:

emspec += amp * np.exp(-0.5 * (self.log10wave-thislinewave)**2 / log10sigma**2) \
            / (np.sqrt(2.0 * np.pi) * log10sigma)

The Gaussian is evaluated at all wavelengths self.log10wave instead of just a few sigma around thislinewave. For earlier desiconda installations using the Intel Math Kernel Library (MKL) 2018.0.1, this wasn't a problem. But newer desiconda installations use MKL 2018.0.2, which is 10x slower for this particular case of evaluating exponentials of large arrays of large negative numbers that will mostly result in 0.

I fixed this by pre-filtering the wavelengths to +/- 5 sigma around the line to add. This restores the performance, and even makes it a bit faster than before.

@moustakas
Copy link
Member

Yowsers!

/ (np.sqrt(2.0 * np.pi) * log10sigma)
jj = np.abs(self.log10wave-thislinewave) < 5*log10sigma
emspec[jj] += amp * np.exp(-0.5 * (self.log10wave[jj]-thislinewave)**2 / log10sigma**2) \
/ (np.sqrt(2.0 * np.pi) * log10sigma)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't check right now if this would ever happen in the code, but you may want to make sure jj isn't sometimes false. Also, can you make allowances for my idiosyncrasies and make it 6-sigma, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I was already being excessive in going to 5 sigma instead of 4 sigma, but ok, I changed it to 6.

Note that jj is an array of booleans, not a single boolean. It is ok even if they are all False. numpy will do the right thing and just not update any wavelengths in that case (i.e. not crash). Does that address your concern, or am I missing something about your "make sure jj isn't sometimes false" suggestion?

x = np.arange(10.0)
y = x**2

#- Some True, some False
ii = x<5
y[ii] += np.exp(-x[ii])

#- All False
jj  = x > 100
y[jj] += np.exp(-x[jj])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. All good, thanks.

@sbailey sbailey merged commit aec05e5 into master Jul 17, 2018
@sbailey sbailey deleted the faster_templates branch July 17, 2018 04:35
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

Successfully merging this pull request may close these issues.

select_mock_targets much slower with desiconda 20180518-1.2.5-spec than 20180130-1.2.4-spec
2 participants