Conversation
This reverts commit 01eace8.
| bp_wave_native = _bandpass_native_waves(newx[in_band]) | ||
| tp_native[in_band] = bandpass._tp(bp_wave_native) / bandpass.wave_factor | ||
| nz = tp_native != 0. # Don't divide by 0. | ||
| assert np.all(newf[~nz] == 0.) |
There was a problem hiding this comment.
It's an temporary compatibility patch, but for clarity an error message can be added something like "thinned sed has non-zero values where throughput is zero"
There was a problem hiding this comment.
Comments on this file should probably be migrated to GalSim-developers/GalSim#1355.
(I no longer have this patch in the PR.) But also, I don't think it's possible for this assert to trigger. That's why it's an assert, not a normal error.
| # Note that this is thinning in native units, not nm and photons/nm. | ||
| interpolant = (self.interpolant if not isinstance(self._spec, LookupTable) | ||
| else self._spec.interpolant) | ||
| newx, newf = utilities.thin_tabulated_values( |
There was a problem hiding this comment.
it can be renamed as thinned_lambda thinned_flux but if this module is going to be removed anyway please ignore this comment
| self.wcs = wcs | ||
| self.pointing = pointing | ||
| if self.wcs is None: | ||
| logger.error("WARNING: wcs and pointing should now be given in process, not fit.") |
There was a problem hiding this comment.
If it's warning, shouldn't it be logger.warning?
If it's an error, the message should be ERROR.
self.pointing is None condition also be added?
There was a problem hiding this comment.
All such messages in Piff are done through the logger mechanism, not python's warnings module. Given that, logger.error vs logger.warning dictates what verbosity level it shows up. logger.error's are always written, which I think makes sense for deprecation warnings. The user should fix it. Warnings that are merely about something going a little weird in the processing, but might be ok are logger.warning.
|
|
||
| def set_context(self, wcs, pointing, bandpass): | ||
| super().set_context(wcs, pointing, bandpass) | ||
| if isinstance(self.components, list): |
There was a problem hiding this comment.
if self.components is not None: can be more straight forward.
There was a problem hiding this comment.
None wouldn't work. It's either a list or an int. The latter is when it is still in the process of being read in from a file, but _finish_read hasn't been called yet. The other check for whether the read is complete uses this, so I'm inclined to keep it this way for consistency.
2 participants
This PR is focused on the implementation of the chromatic=True option for RomanOptics. It includes the following pieces:
The config file in devel/roman using chromatic=True is about 5-6 times slower than the achromatic version. This is with just a linear approximation to the SED (sed_max_samples=2) -- it's almost double that with 3 samples. This seems reasonable enough given the extra work intrinsic to drawing chromatically, but there might be further optimizations available to try. E.g. I haven't tried using photon shooting (for either one), which might be worth trying despite the fact that it results in noisy images (which makes the empirical derivatives tricky, but there might be ways around that). I also don't have a good feel for how much the different SED approximations matter in practice, since we haven't tested yet with realistic SEDs.
I'll also reiterate that even with chromatic=False, the final PSF model is chromatic. Just the fitting is done achromatically with the PSF being evaluated at just a single wavelength (the bandpass effective wavelength). I'm open to adjustments to the naming of this parameter, since it's potentially confusing as it currently stands.