-
Notifications
You must be signed in to change notification settings - Fork 39
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
Tickets/sp 1908 #440
base: main
Are you sure you want to change the base?
Tickets/sp 1908 #440
Conversation
@@ -102,7 +111,13 @@ def run(self, data_slice, slice_point=None): | |||
idx_template_inputs = ok_template_input[: self.n_images_in_template] # Images included in template | |||
|
|||
Night_template_created = data_slice[self.night_col][idx_template_created] |
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.
While we're here -- this seems wrong, with a capital letter at the start of the variable?
@@ -102,7 +111,13 @@ def run(self, data_slice, slice_point=None): | |||
idx_template_inputs = ok_template_input[: self.n_images_in_template] # Images included in template | |||
|
|||
Night_template_created = data_slice[self.night_col][idx_template_created] | |||
N_nights_without_template = Night_template_created - data_slice[self.night_col][0] | |||
N_nights_without_template = Night_template_created |
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.
This seems incorrect - n_nights_without_template should not be the same as the night_template_created.
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.
n_nights_without_template could be counted from the start of the survey, or it could be counted from the first observation in this field. Either would be valid.
Or it could be dropped, given that "night_template_created" is present, as well as n_images_until_template".
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.
For me, I think n_night_without_template makes the most sense as number of nights without a template image when there could have been a template image. But that's trickier to compute. Probably easiest to drop it.
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.
Do you want to go ahead and drop it? I think this is reasonable.
np.degrees(slice_point["ra"]), | ||
data_slice[self.mjd_col][idx_template_created], | ||
mjd_start=self.mjd_start, | ||
) |
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.
How is this the fraction of the season, if its' just calc_season? (calc_season will return the integer + fractional season)
@@ -22,12 +23,14 @@ class TemplateTime(BaseMetric): | |||
Default 3. | |||
seeing_percentile : `float`, opt | |||
Maximum percentile seeing to allow in the qualified images (0 - 100). | |||
Default 50. | |||
XXX--deceptive kwarg since it checks the percentile at each point on the sky |
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 really don't know if per-point-on-the-sky was the intended plan or not. I kind of think it was, so not necessarily deceptive but could be clarified (it also reads backwards to me)
"Max percentile of seeing values (at each RA/Dec) to allow in the template source images (0-100)".
It's also worth checking with Eric if the default should be 50 or 100.
Previously we had a hard cutoff, which wouldn't be the worst way to go either (could potentially have both kwargs and use one to override the other).
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.
Pinging @ebellm for feedback on the update to the default 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.
Doing a percentile cut per-point-on-sky seems very wrong to me since then you could have 5 images with fantastic seeing, but it'll say it can't make a template because their aren't 3 over 50th percentile.
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 can see that .. and vice-versa if you have a spot on the sky with only terrible seeing, you might want to make a template anyway.
I wonder if the actual way to do it is to mix the two together - if you have X images better than some (high percentile over the whole sky) value, make the template and if you have X images better than the 50% level at this point in the sky, make the template anyway as well.
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.
You are both right, this is tricky--you always want the best images, but what counts as "good enough" does have some dependence on the minimum airmass you can observe the field at, for instance. I had though to use percentiles to avoid undue assumptions about the shape of the seeing distribution, but a hard cut to avoid the worst seeing tail + a local percentile might be a decent approach?
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 we're mostly concerned that by saying a local percentile only, that you will always require 5 images before you can make a template, and that clearly shouldn't be the case in operations in the first year.
We do have a way to calculate the minimum airmass a point could have (the point on the sky under consideration in the metric), so we could scale a minimum seeing requirement for the declination of the current point --
then we could have a "good seeing for this declination" threshold, if you have 3 images that meet that, then go make a template -- and otherwise, wait for 3 images which are better than the 50%tile.
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.
Sounds fine to me.
Default 50. | ||
XXX--deceptive kwarg since it checks the percentile at each point on the sky | ||
and not continuously. | ||
Default 100. |
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.
Likewise as above.
Some minor updates to make the incremental template metric clearer.