-
Notifications
You must be signed in to change notification settings - Fork 1
synspec #17
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
base: master
Are you sure you want to change the base?
Conversation
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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
Copying some questions from Slack:
I decided not to put it there because I couldn't figure out how to get
Nope - |
pllim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synspec_examples.ipynb
Outdated
| "name": "stderr", | ||
| "output_type": "stream", | ||
| "text": [ | ||
| "WARNING: W35: None:5:0: W35: 'value' attribute required for INFO elements [astropy.io.votable.tree]\n", |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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'): |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 / dwAnd 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'): |
There was a problem hiding this comment.
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?
A place for comments on "synspec" - simulating spectroscopy with synphot tools.