tickets/SP-2957: Support for pre-survey-start progress reports#193
Conversation
yoachim
left a comment
There was a problem hiding this comment.
Looks good. As usual, I'd like to refactor maf to get rid of MetricBundle and MetricBundleGroup since that would make things easier to read here.
| metric_values = [] | ||
| name = None | ||
| mjds_with_data = [] | ||
| for mjd in mjds: |
There was a problem hiding this comment.
This is a bit of a dangerous loop because it's not going to scale well as you increase the number of mjds. Usually you're going to be better off using a VectorMetric since that avoids the repeat calculations that happen here. Of course there are some metrics that are complicated enough you just have to brute force it like this.
There was a problem hiding this comment.
In one of my example metrics (the simple count of visits in one band), I think I see how to use a vector metric (AccumulateCountMetric), but I don't see a way to do it for the other one (fOArea). What I probably want is to create a separate compute_mjd_vector_metric that returns the same series as compute_scalar_metric_at_mjds so we can do the fast thing when there is a vector metric, and use the scalar metric when it isn't possible (or just not implemented yet).
|
|
||
|
|
||
| def compute_mixed_scalar_metric( | ||
| start_visits: pd.DataFrame, |
There was a problem hiding this comment.
I probably would have just gone with passing in visits rather than start_visits and end_visits and then slice the visits is iloc rather than having to concat.
And this is another loop where it looks like you're going to end up doing a lot of repeat calculations--there should be some VectorMetrics that accumulate values which I think is what you want here.
There was a problem hiding this comment.
I'm confused about what you're suggesting here. You mean that the caller should concatenate the consdb and baseline visits together into a single df before calling this function, and this function should use indexing to select the needed subsets?
There was a problem hiding this comment.
And this is another loop where it looks like you're going to end up doing a lot of repeat calculations--there should be some VectorMetrics that accumulate values which I think is what you want here.
I agree that we should be able to optimize this. For simple additive metrics (e.g. total teff or total number of visits accumulated), we should be able to get the value out of the baseline and completed values directly, and not need to recompute the metrics at all:
df.chimera[mjd] = df.completed[mjd] + df.baseline[end_mjd] - df.baseline[mjd]
For things that aren't simply additive, it's less obvious to me how to do it.
There was a problem hiding this comment.
Ah, this is for mashing as-observed and sims in the future, then having start_visits and end_visits is good.
| return cmap | ||
|
|
||
|
|
||
| LIGHT_PLOT_BAND_COLORS = { |
There was a problem hiding this comment.
In the past I've used f2c = {'u': 'purple', 'g': 'blue', 'r': 'green', 'i': 'cyan', 'z': 'orange', 'y': 'red'} to set color per band. There really ought to be a project standard.
It might be nice (probably on another ticket), to put this somewhere like rubin_scheduler.utils.plotting_utils, so we can have easy standard colors across packages at least.
There was a problem hiding this comment.
There is, in fact, a project standard (here), which is where I got the colors I used.
There didn't used to be a module that supplied the palettes, but looking for the reference just now I see that there is (lsst.utils.plotting.get_multiband_plot_colors). I'll still need to add some by hand to have colors for things like no filter, pinhole, etc.
There was a problem hiding this comment.
It also looks like the recommended palette changed again.
There was a problem hiding this comment.
I really need to get lsst.utils into conda-forge so we can depend on it .. it's just that when we do depend on it via condo, we also have to make sure that the version of rubin_sim/scheduler/schedview that is part of rubin-env does NOT depend on it (so as to not shadow the project eups-version lsst.utils when they build rubin-env).
There was a problem hiding this comment.
I pushed a change that uses lsst.utils if it's there, and falls back on values copied from the current version if it is not. So, we should be able to use it as is, and once lsst.utils gets into a state where we declare it as a dependency, this use of it will automatically start using it without additional changes.
| column (named by ``name_col``) identifying which pointing each visit | ||
| matches to. The returned DataFrame maintains the original visit data | ||
| but is filtered to only include visits that match to at least one | ||
| pointing. |
There was a problem hiding this comment.
Would be good to mention what the behavior is for multiple matches--do we get all the matches, the first match, etc?
|
I also just caught a bug on my own while addressing the comment Lynne had about the use of starting MJD in the tests: in the current version in git, it assumes that the latest visit in completed visits is on the latest night of observing. But, this isn't necessarily the case, it could be, for example, that the last few days were cloudy and there was no observing. The plots should still count these nights as "completed," even though there are on visits on them. |
rhiannonlynne
left a comment
There was a problem hiding this comment.
I had two comments that are not outdated but got stuck in pending so this might push them through.
|
|
||
| if len(summary) > 0: | ||
| fig_html = f"<details><summary><b>{summary}</b></summary>{fig_html}</details>" | ||
|
|
There was a problem hiding this comment.
Oh interesting .. can you add "alt" text here as well?
I don't know what you were planning to put into the summary text, but the alt text could be different if the summary is intended to be higher level (the alt text could for example be a description of the metric specifically and the slicer, and then what kind of plot is created from that data .. so a healpix skymap of the number of visits could translate to
"A showing calculated over , after subselecting data via ."
it's not as good as something that would also describe the kinds of points or what the data show, but that's a bit beyond what we could generate automatically anyway.
There was a problem hiding this comment.
The "summary" text is what is shown when section is collapsed, so it's not really a literal "summary". For example, the heading of the hourglass plot for one value for one month such as "Hourglass of Hour Angle for April, 2025".
Adding values to the alt text is easy as well: any html can go here. As you point out, coming up with a way to generate automatic text that goes into this html tag that conveys the same information as an hourglass plot but through a screen reader is, however, a significant challenge. I'll take a look around for best practices for providing alt text for automatically generated plots.
There was a problem hiding this comment.
So -- I know it may be confusing, but MAF when you generate a plot through the metric bundle -- will actually auto-generate a caption that contains the background information about how the plot was generated, like it can auto-generate a name for the output and title and axes labels for the plots.
This is not the same as the information captured in the plot itself (i.e. whether there was a trend upward or downward). I'm not sure which one you meant here.
There was a problem hiding this comment.
My understanding is that the alt text is supposed to serve as a substitute for seeing the image, so that people using screen readers or text-only browsers can use the page. See the official standard here, which states directly that:
the alt attribute's value should never contain text that could be considered the image's caption, title, or legend. It is supposed to contain replacement text that could be used by users instead of the image; it is not meant to supplement the image. The title attribute can be used for supplemental information.
So, we could add the text generated by MAF as a title attribute, but generating text for the alt attribute for an automatically generated plot is a more challenging problem.
Reading recommendations on alt text, it both needs to be short (so someone using a screen reader doesn't need to sit through overly detailed descriptions) and a useful alternative to seeing the figure (its official purpose). For information rich visualizations, these goal conflict, and the gist of the recommendations I've found in my web searching this morning and yesterday (e.g. here) is just to state in words the most important trends and relationships shown by the figure. I'm having trouble finding recommendations for how to do this for automatically generated figures.
To balance brevity with being a useful substitute, maybe we can choose one or two meaningful maf summary metrics for the metrics shown in each figure, and make the alt text one or two sentences saying what the values of these summary metrics are.
| return cmap | ||
|
|
||
|
|
||
| LIGHT_PLOT_BAND_COLORS = { |
There was a problem hiding this comment.
I really need to get lsst.utils into conda-forge so we can depend on it .. it's just that when we do depend on it via condo, we also have to make sure that the version of rubin_sim/scheduler/schedview that is part of rubin-env does NOT depend on it (so as to not shadow the project eups-version lsst.utils when they build rubin-env).
There are (or will be) corresponding PRs on rubin_sim and schedview_notebooks.