Skip to content
This repository was archived by the owner on Feb 27, 2023. It is now read-only.

Conversation

@spletts
Copy link

@spletts spletts commented Oct 1, 2019

Makes a table of synthetic magnitudes and regressed magnitudes

Copy link
Member

@djperrefort djperrefort left a comment

Choose a reason for hiding this comment

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

Overall this is great progress and looks really good. The missing transmission filter and the use of the AB offset may be the source of the problem. I also included general comments I thought you might find useful.

import sndata
from sndata.csp import dr1, dr3
from .. import lc_colors
from .. import spectra_chisq
Copy link
Member

Choose a reason for hiding this comment

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

You have multiple unused imports here that can be removed. You only need

import numpy as np
import sncosmo
import sndata
from astropy import constants as const
from astropy import units as u
from astropy.table import QTable, Table, join, unique, vstack
from sndata.csp import dr1, dr3

from .. import lc_colors

obj_id = spec_data.meta['obj_id']

# Add appropriate units
spec_data['wavelength'] *= u.angstrom
Copy link
Member

Choose a reason for hiding this comment

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

In general it is safer to write this as

if not spec_data['wavelength'].unit:
    spec_data["wavelength"].unit = u.angstrom

@@ -0,0 +1,196 @@
"""Create table with synthetic magnitudes and regressed magnitudes.
Copy link
Member

Choose a reason for hiding this comment

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

As a general note, I would suggest taking a quick look at PEP8 which is the formal python style guide. There are various tools and IDE's that can do this automatically for you (I find that doing it manually is not realistic for most projects.)

"""

# Create quantity table to correctly handle squaring units
# http://docs.astropy.org/en/latest/table/mixin_columns.html#quantity-and-qtable
Copy link
Member

Choose a reason for hiding this comment

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

+1 for the inclusion of a link that describes the issue!

gauss_process = lc_colors._lc_regression.fit_gaussian_process(data=photo_data)
# Predict photometric data at the spectra times for specific band
# Units of Jy?
pred_flux, pred_flux_err = lc_colors._lc_prediction.predict_band_flux(gp=gauss_process, band_name=band, times=spec_times)
Copy link
Member

Choose a reason for hiding this comment

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

lc_colors._lc_prediction.predict_band_flux -> lc_colors.predict_band_flux

return out_table


def make_table():
Copy link
Member

Choose a reason for hiding this comment

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

This function looks like a duplicate of the run_phot_comp.make_table function.

# TODO display filename at prompt?
ans = input('Overwrite joint table? (y/n)')
if ans == 'y':
tbl.write('../../../../comp.fits', overwrite=True)
Copy link
Member

Choose a reason for hiding this comment

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

Relative file paths like this can be tricky. They depend on the directory where you are running the code from, which is not always the same as the file's parent directory. A more robust way is to use

from pathlib import Path

base_dir = Path(__file__).parent

# Use however many parents you need
out_path = base_dir.parent.parent.parent / 'comp.fits'

'calc_chisq',
'tabulate_chisq'
'tabulate_chisq',
#'photometry_to_spectra_time',
Copy link
Member

Choose a reason for hiding this comment

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

You don't want these commented out. The __all__ attribute defines what is imported when you run from analysis.spectra_chisq import *. However, importing * is almost always frowned upon. We use it here because the automatic documentation builder requires it to know what functions to include in the online docs.

flux: F_nu; flux per frequency. Units: Jansky
"""

flux = 10**(-0.4 * (mag - zp - offset))
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be flux = 10**(-0.4 * (mag + offset - zp))

@djperrefort
Copy link
Member

@spletts I talked with a friend here in Pasadena and she recently was doing something similar. I've pushed a change that borrows her code snippet. Some targets like 2005kc look pretty great, others like 2004ef and 2004dt don't. However, I think the results are very close to what we want and are mostly correct. I would look at plots of a few other targets, pick a few that look good/bad and discuss it with Michael at group meeting.

@spletts
Copy link
Author

spletts commented Nov 13, 2019

Calculates synthetic photometry using the technique in Casagrande & VandenBerg 2014.

@djperrefort djperrefort dismissed their stale review November 20, 2019 20:11

Old review - out of date

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants