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

force wavelenth arrays to be idenical for twilight solar spectra #375

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

yoachim
Copy link
Member

@yoachim yoachim commented Oct 18, 2023

Quirks in Bandpass and Sed, so adding a line to prevent extra resampling of an Sed object.

@rhiannonlynne
Copy link
Member

Where is the solar spectra coming from? (usually kurucz models aren't uniformly sampled, so I'm wondering where this is coming from).

@rhiannonlynne
Copy link
Member

rhiannonlynne commented Oct 19, 2023

I didn't dig through the code to see how this affects the problem Mike specified .. it seems like it's a cross-platform issue that was the cause of the problem for them (i.e. something like "1100 != 1100"??). I'm not entirely sure if this is going to avoid the problem ... and honestly, I'm not sure why it's happening.
What's actually happening in the code is this:

  • the cannon and other filters are being read in and resampled onto a regular grid
  • the solar spectrum is being read in (and stays on its own grid)
  • when the magnitude for the solar spectrum is calculated in any bandpass, it's resampled onto that regularly gridded bandpass (inside "calc_flux") if necessary (and here, machine precision may mean that it's called anyway).
    What's funny is that "calc_flux" calls Sed's resample_wavelength using the wavelength grid of the filter already .. so wavelength_grid should == wavelength_match and the solar spectrum should extend beyond the solar sed anyway.

The way to really truly avoid the nan currently is to read in a solar sed that extends beyond the expected range of the wavelengths for the filters, including the Canon filter. What's odd, is this seems to be likely already true?
*** OH -- yeah, ok, so this is the problem (I finally unpicked the rest of the steps around this code). You're resampling the Canon filters onto the solar spectrum wavelength range, but you should just pick a useful range for these. .. why not 350 to 800 nm where they all are well cut off? The solar sed ranges from 300.0 to 2499.999999999758 and it's likely the precision for this long-wavelength value that's causing the problem.

I'm also going to put in a PR to change the "use a nan" to "use the specified fill value" and set that to default to 0. I'll also look around for some other places to simplify what we're doing, as it just got so complicated with extra calls when it was in use for catalogs. this will help prevent this in the future, but for now -- instead of resampling using a different wavelength step, please resample using a more limit wavelength range.

Copy link
Member

@rhiannonlynne rhiannonlynne left a comment

Choose a reason for hiding this comment

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

It would be better and more robust to reduce the wavelength range for the Canon filters to stay within the wavelength range of the solar spectrum.

@@ -381,8 +381,11 @@ def __init__(self, mags=False, dark_sky_mags=None, fit_results=None):
bp_temp.resample_bandpass(
wavelen_min=self.solar_spec.wavelen.min(),
wavelen_max=self.solar_spec.wavelen.max(),
wavelen_step=0.2,
wavelen_step=self.solar_spec.wavelen[1] - self.solar_spec.wavelen[0],
Copy link
Member

Choose a reason for hiding this comment

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

The problem isn't here, it's that wavelen_min and wavelen_max should be within the range of the solar spectrum you're using. The step being 0.2 is fine.

)
# Force wavelengths to be identical so
# it doesn't try to resample again later
bp_temp.wavelen = self.solar_spec.wavelen
Copy link
Member

Choose a reason for hiding this comment

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

If the solar spectrum had an irregular grid, this would be incorrect, so it's probably not a good example to set. (it's also worth noting that if I do "np.diff" on the solar spectrum wavelength array, I do get a few different values .. that look the same, but I'm assuming this machine precision stuff).

@rhiannonlynne
Copy link
Member

If you could please also run ruff and address the errors in this file before closing the PR, that would be great.

@yoachim yoachim merged commit c3affd5 into main Oct 19, 2023
7 of 9 checks passed
@rhiannonlynne rhiannonlynne deleted the u/yoachim/sed_twilight branch October 20, 2023 20:49
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.

2 participants