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

Improve behavior of SED class, especially with respect to pickling and spline interpolation. #1257

Merged
merged 17 commits into from
Oct 30, 2023

Conversation

rmjarvis
Copy link
Member

@rmjarvis rmjarvis commented Oct 27, 2023

This PR addresses a number of things that have turned up in various imsim/roman_sim runs related to the SED classes. The initial impetus for this work was to let the SEDs we get from skyCatalogs to be picklable, so image.nproc would work correctly, and other use cases as well would not be quite so finicky about pickling.

  • The main solution for this problem was to have mul_sed try to return a tabulated sed if possible. I.e. when either (or both) of them uses a lookup table.
  • Another thing that helps for some cases is to not try to pickle the derived SEDs in chromatic objects that have the SED as a lazy_property. It's not very inefficient to remove them from the dictionary before pickling and remake them if needed. (Indeed it might be usually more efficient, since there is less serialization and communication involved.)
  • While I was at it I addressed issue Revisit use of spline LookupTable in SED class #1187, making it generally fine to use spline interpolation for Bandpass and SED objects. The default in these classes is still linear (e.g. when reading from a file), but now it's not inefficient to build an SED with a regular LookupTable using the default interpolant='spline'.
  • Given the above, I added an interpolant option to these classes so if you want to read from a file using spline interpolation, it is now possible.
  • I added a clip_neg option to DistDeviate to clip any negative values in the probability distribution to 0. This is now used in sampleWavelengths, since spline interpolations can lead to slightly negative values in some cases. Rather than try to avoid it in the SED class, it seems more sensible to just allow DistDeviate to gracefully ignore those negative values. The default behavior of DistDeviate is unchanged -- with the default clip_neg=False, it raises an error if there are any negative probabilities.
  • The hardest thing for getting spline to work well was to get the thin() algorithm to work correctly with a spline table. I couldn't find an O(N) algorithm for that, so it uses the slower algorithm, but I think it's basically working now.
  • Along the way I found two bugs in the new EmissionLine class, which I fixed. First, the atRedshift function was wrong. It both set the redshift value and changed the wavelength of the line. The nominal wavelength shouldn't change, since the given spectrum is supposed to define the rest-frame SED, not the observed SED.
  • Also, in some use cases, the 0 at the edge of the emission line could turn into a 1.e-16 kind of number. When integrating from there to "infinity", that could lead to crazy bolometric fluxes. My fix for this was to add a second 0 another fwhm away on either size to really force it to zero when not near the line.
  • Related to that, I changed out definition of infinity in bolometric fluxes from 1.e100 to 1.e30 (nm). That's still 1.e21 meters, which is way past any conceivable radio wave wavelength. But it allows functions of wave with some modest power like wave**3 to not blow up.
  • Finally, I added an interpolant option to our trapz function to make it easier do the integral in the thin_tabulated function using the specified interpolant. I didn't end up using it in as many places as I originally expected to, but still, I think it's a useful enough feature, so I kept it.

@rmjarvis rmjarvis added this to the v2.5 milestone Oct 27, 2023
@rmjarvis rmjarvis added chromatic Related to the Chromatic classes, SEDs, bandpasses, etc. optimization/performance Related to the speed and/or memory consumption of some aspect of the code labels Oct 27, 2023
Copy link
Member

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for catching the EmissionLine bugs. Just one small question.

galsim/sed.py Outdated
@@ -413,7 +421,8 @@ def _fast_spec(self):
f = self._rest_nm_to_photons(x)
else:
f = self._rest_nm_to_dimensionless(x)
return _LookupTable(x, f, interpolant='linear')
interp = self._spec.interpolant if isinstance(self._spec, LookupTable) else 'spline'
Copy link
Member

Choose a reason for hiding this comment

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

Most of the other default interps look to be 'linear', why 'spline' here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I was originally a little aggressive about trying to get most things to use spline by default, but then I backpedaled on that idea, since it felt a little too API-changing. I just missed this one.

@rmjarvis rmjarvis merged commit 1b56bb4 into releases/2.5 Oct 30, 2023
10 checks passed
@rmjarvis rmjarvis deleted the sed_mul_table branch October 30, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromatic Related to the Chromatic classes, SEDs, bandpasses, etc. optimization/performance Related to the speed and/or memory consumption of some aspect of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants