Skip to content

Conversation

@tcjansen
Copy link
Owner

A place for comments on "synspec" - simulating spectroscopy with synphot tools.

tcjansen and others added 25 commits June 4, 2019 16:12
Co-Authored-By: P. L. Lim <2090236+pllim@users.noreply.github.com>
Co-Authored-By: P. L. Lim <2090236+pllim@users.noreply.github.com>
… out binset and forced tapering in Observation calls
@@ -1 +0,0 @@
Browse the branches to see the different notebooks I've been working on! No newline at end of file
Copy link

Choose a reason for hiding this comment

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

Why do I seem to see this diff in multiple PRs? Is this intentional?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, that's because I don't copy the README over to the new branches

@tcjansen
Copy link
Owner Author

tcjansen commented Jul 15, 2019

Copying some questions from Slack:

wl_in_center = ((1.1 - 0.33) / 2 + 0.33) -- why not apply u.micron here instead of later?

I decided not to put it there because I couldn't figure out how to get np.arange() to play nicely with unit quantities in this line:
spec_wavelengths = np.arange(0.33, 1.1 + delta_wavelength, delta_wavelength) * u.micron

For a bandpass that is equivalent with "observing through no filter", why not use Const1D with amplitude=1? Does it have to be a box?

Nope - Box1D was just the first model I saw that worked :P. But Const1D will work better, thanks.

Copy link

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Some minor comments. I'll defer to @bmorris3 and @eteq for the science side of things.

"name": "stderr",
"output_type": "stream",
"text": [
"WARNING: W35: None:5:0: W35: 'value' attribute required for INFO elements [astropy.io.votable.tree]\n",
Copy link

Choose a reason for hiding this comment

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

These warnings show disappear if you use yet-to-be-released astropy 4.0 😬 . In the meantime, if they bother you, you can filter them out with https://docs.python.org/3/library/warnings.html#overriding-the-default-filter .

binset = np.array([first_wl])
binwidth = first_wl / R

while binset[-1] <= waverange[-1].to(u.angstrom).value:

Choose a reason for hiding this comment

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

I haven't though very hard about this yet, but is there a way to write this without a while loop, to keep it speedy?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will think more about this!

synspec.py Outdated
Returns a spectrum with bins defined by a constant bin width specified
by the user.
"""
if not waverange:

Choose a reason for hiding this comment

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

I think it's a little clearer what you mean if you write this as if waverange is None:

import numpy as np


def from_R(source_spectrum, bandpass, R, waverange, force='none'):
Copy link

Choose a reason for hiding this comment

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

R is also another high level programming language. And typically uppercase variable signifies a constant, so I propose renaming R to resolution, spectral_resolution, or something like that.

binset = np.array([first_wl])
binwidth = first_wl / R

while binset[-1] <= waverange[-1].to(u.angstrom).value:
Copy link

Choose a reason for hiding this comment

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

First of all, I don't think this can be vectorized easily as binset[i] depends on binset[i - 1]; and also we don't have to worry about speeding it up until it is proven to be a bottleneck; and even then Cythonizing it might be good enough.

However, I am concerned that this algorithm does not round-trip. For example:

waverange = [5000, 5500] * u.AA
R = 100 

Following the math, I think binset here is actually bin edges. And I think you mean in your docstring that you want the bin's central wavelength divided by its bin width is R, correct? So, having calculated binset with this algorithm, my attempt to round trip as follows:

dw = binset[1:] - binset[:-1]  # Bin widths
wcen = binset[:-1] + dw / 2  # Recalculate bin centers
observed_R = wcen / dw

And I get 200.5, not 200.

So then, I tried to calculate the bins by hand, knowing the starting wavelength and R (knowing ending wavelength does not help much at the start here, but it lets you know when to stop the bins). However, I got stuck after the first bin. This is because, given that every bin is defined as wcen / dw = R, and I know the edge of the first bin, for the second bin, I know neither the wcen nor its dw in advance, and it seems to me that I have to solve for two unknown at once. Am I going crazy here?

Then I looked around in specutils and only found spectral_resolution() that calculates the R given the wavelengths, but not the other way around. This makes me think that maybe requiring R to be constant throughout is not practical in the first place?

🤔

return Observation(source_spectrum, bandpass, binset=binset, force=force)


def from_wave_array(source_spectrum, bandpass, waveset, force='none'):
Copy link

Choose a reason for hiding this comment

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

I don't see from_wave_array being used in the notebook, is this function necessary?

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.

4 participants