Model spectra #34
Model spectra #34
Conversation
…unnecessary functions and parameters
djperrefort
left a comment
djperrefort
left a comment
There was a problem hiding this comment.
This is a good step in the right direction. The new code is a little cluttered, and it's hard to follow since there is a lot of missing or incorrect docs. This should be merged in. Just take an hour or so to update the docstring and clean up the comment lines first.
| from scipy.optimize import curve_fit | ||
| from sndata.csp import DR1, DR3 | ||
|
|
||
| from astropy.table import join, vstack #unique |
| return time + 2400000.0 | ||
|
|
||
|
|
||
| def get_jd_t0(obj_id, tbl_num=3, mjd=True): |
There was a problem hiding this comment.
tbl_num and mjd should not be arguments. This function won't work correctly with other table numbers or date formatting.
| return t0 | ||
|
|
||
|
|
||
| def create_model(model, obj_id, a_v, redshift, amplitude, r_v=3.1): |
There was a problem hiding this comment.
The name of this function is misleading and the docstring is incorrect. Generally speaking, this function is a small wrapper for model.set and is unnecessary. Eliminate this function.
| return model | ||
|
|
||
|
|
||
| def get_model_flux(wave, amplitude, a_v, obj_id, time, model, redshift): |
|
|
||
| from astropy.table import join, vstack #unique | ||
|
|
||
| #from ._synthetic_photometry import get_synth_photo_for_id # FIXME uncomment if fit performed using photometry |
There was a problem hiding this comment.
You have a lot of commented out code. Try to avoid this. Use comments to increase clarity of what the code is doing. I find some of the code in this file hard to follow because all the commented out code makes it cluttered.
|
|
||
| def get_model_synth_mag(time, amplitude, a_v, obj_id, wave, model, redshift, spec_release, phot_release, temp_spec_tbl, spec_data): | ||
| """ | ||
| :param time: |
|
|
||
|
|
||
|
|
||
| def fit_band_to_obs_photo(): |
There was a problem hiding this comment.
When a placeholder function isn't necessary to execute other code, it's usually better to make a single Todo list at the top of the file. This is less cluttered.
| # Note: order of args matters in `get_model_flux()` | ||
| if fit_u_band: | ||
| fit_params, cov_matrix = curve_fit(partial_model_flux, wave, flux) | ||
|
|
There was a problem hiding this comment.
else:
raise NotImplimentedError('Some descriptive message')| This function is used with ``_model_spectra`` module to fit extinction parameters using synthetic photometry and | ||
| observed photometry. | ||
| Args: |
There was a problem hiding this comment.
Missing docs. In particular, I'm confused what temp_flux is.
| #spec_data = unique(spec_data, keys='time') | ||
|
|
||
| # TODO only one band is used in temp_flux.... change this. Want one fit for all bands? | ||
| if temp_flux is not None and temp_spec_tbl is not None: |
There was a problem hiding this comment.
What are you trying to do here? I'm not clear why temp_flux and temp_spec_tbl are necessary?
|
@spletts Just wondering if you've made any progress on this PR? |
Fixed redshift problem -- the wavelengths from DR are in the rest frame. The function
fit_to_obs_spectra()is the only one that needs reviewing (for now). The other functions have either already been reviewed or are not yet ready for review.