Skip to content

Conversation

O957
Copy link
Collaborator

@O957 O957 commented Aug 5, 2025

For the full scope of this PR, please refer to issue #117 .

Specifically, this PR adds:

  • Legends to the plot with labels for forecasts and observations, when both are present.

@O957 O957 added this to the [July 28, August 8] milestone Aug 5, 2025
@O957 O957 self-assigned this Aug 5, 2025
@O957 O957 linked an issue Aug 5, 2025 that may be closed by this pull request
@O957
Copy link
Collaborator Author

O957 commented Aug 11, 2025

This is not gated on #125 and #18 but it would be nice if these were complete before proceeding further on this PR.

@O957
Copy link
Collaborator Author

O957 commented Aug 12, 2025

On faceted charts, do we want the legend under each plot? By default, if not under each plot, the legend appears all the way at the bottom of the faceted plots.

Screenshot 2025-08-12 at 16 57 43

@dylanhmorris
Copy link
Collaborator

dylanhmorris commented Aug 13, 2025

I prefer, in order:

  1. One legend for all subplots located above the top-most subplot
  2. One legend for each subplot located above that subplot
  3. One legend for each subplot located below that subplot (as depicted in @AFg6K7h4fhy2's post)

@O957
Copy link
Collaborator Author

O957 commented Aug 13, 2025

The problem I am having is when I do independent domain and range for alt.Color encoding arguments, the outcome is that, when both files are uploaded, the Observations color is overriden and turns from limegreen to steelblue. When I use the system I currently have in place, all labels are their proper color (well, the CI labels to not seem to be the correct opacity) but appear even if the data is not present (Observations legend label present even when just uploading the forecast file).

Screenshot 2025-08-13 at 14 22 43

I tried querying AI somewhat on this but it wasn't able to help really and propered solutions that did not work / were seemingly not sensible.

I will keep at it.

@damonbayer
Copy link
Collaborator

The problem I am having is when I do independent domain and range for alt.Color encoding arguments, the outcome is that, when both files are uploaded, the Observations color is overriden and turns from limegreen to steelblue. When I use the system I currently have in place, all labels are their proper color (well, the CI labels to not seem to be the correct opacity) but appear even if the data is not present (Observations legend label present even when just uploading the forecast file).

Screenshot 2025-08-13 at 14 22 43 I tried querying AI somewhat on this but it wasn't able to help really and propered solutions that did not work / were seemingly not sensible.

I will keep at it.

The legend doesn't really add much to the plot, in my opinion. I think we should abandon the PR, if it is proving difficult.

@damonbayer
Copy link
Collaborator

I don't think there is any way for the transparencies to match the legend. By definition the 50% CI is overlayed on top of the the 90%, etc. The legend will have no concept of this.

We could swap to different colors for each band to resolve this. To mimic the R plots, use #DEEBF7, #9ECAE1, and #3182BD.

@O957
Copy link
Collaborator Author

O957 commented Aug 13, 2025

We could swap to different colors for each band to resolve this. To mimic the R plots, use #DEEBF7, #9ECAE1, and #3182BD

Sounds great.

This commit 7185da2 was clean and had Forecast in steelblue wo/ actual CI labels, but what you just suggested sounds better.

@damonbayer
Copy link
Collaborator

damonbayer commented Aug 13, 2025

@AFg6K7h4fhy2 I suggest getting the colors programmatically with

>>> import colorbrewer
... colorbrewer.Blues[3]
[(222, 235, 247), (158, 202, 225), (49, 130, 189)]

@O957 O957 marked this pull request as ready for review August 14, 2025 14:02
@O957 O957 requested a review from dylanhmorris as a code owner August 14, 2025 14:02
@O957 O957 requested a review from damonbayer August 14, 2025 14:38
labels = [f"{round((high - low) * 100)}% CI" for low, high in ci_pairs]

palette_rgb255 = list(
colorbrewer.Blues[max(3, min(MAX_NUM_CIS, len(labels)))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAX_NUM_CIS is not used to limit ci_pairs. Seems like an oversight.

palette_rgb255 = list(
colorbrewer.Blues[max(3, min(MAX_NUM_CIS, len(labels)))]
)
palette = [(r / 255, g / 255, b / 255) for r, g, b in palette_rgb255]
Copy link
Collaborator

@damonbayer damonbayer Aug 14, 2025

Choose a reason for hiding this comment

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

This approch, using rgb_strings, feels more straightforward than division + hex conversion.

import altair as alt
import polars as pl
import colorbrewer

rgb_tuples = colorbrewer.Blues[3]
rgb_strings = [f"rgb({r},{g},{b})" for r, g, b in rgb_tuples]

df = pl.DataFrame({
    'x': [1, 2, 3],
    'y': [4, 5, 6]
})


chart = alt.Chart(df).mark_point(
    color=rgb_strings[2],
    size=100
).encode(
    x='x',
    y='y'
)

chart

Comment on lines 196 to 198
"low": f"{low:.3f}".rstrip("0").rstrip("."),
"high": f"{high:.3f}".rstrip("0").rstrip("."),
"color": to_hex(color),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, let's use format_percent from babel.

Comment on lines 405 to +408
bands = [
band("0.025", "0.975", 0.10),
band("0.1", "0.9", 0.20),
band("0.25", "0.75", 0.30),
band(spec["low"], spec["high"], label)
for label, spec in ci_specs.items()
if spec["low"] in df_wide.columns and spec["high"] in df_wide.columns
Copy link
Collaborator

@damonbayer damonbayer Aug 14, 2025

Choose a reason for hiding this comment

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

Structure overall feels a bit off.

What about

  1. We haveget_available_ci(list_of_quantiles) which returns all of the available ci's (width only, or width, lower, and upper)
  2. Then somewhere later we have the availability to subsample the available ci's (either by giving the list of widths we want or by limiting to some max number).
  3. Then we get the colors based on the length of the subsampled list of ci's.
  4. Then we make the plot with legend.

Copy link
Collaborator Author

@O957 O957 Aug 14, 2025

Choose a reason for hiding this comment

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

I will try this out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Get the colors" step may be obviated by https://vega.github.io/vega/docs/schemes/#blues

from typing import Literal

import altair as alt
import colorbrewer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than colorbrewer, we should try to take advantage of https://vega.github.io/vega/docs/schemes/#blues

Copy link
Collaborator

@damonbayer damonbayer left a comment

Choose a reason for hiding this comment

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

The output looks good, but I think we can make the internals better.

]

num_colors = max(3, min(MAX_NUM_CIS, len(labels)))
blues_map = colormaps["Blues"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the plan was to use vega's built-in implementation of the blues palette. Is there some reason that doesn't work? https://vega.github.io/vega/docs/schemes/#blues

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legend In Plots

3 participants