Skip to content

Support interval+reduce in the area mark #2329

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Support interval+reduce in the area mark #2329

wants to merge 3 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 6, 2025

Defaults y to the reducer when creating a dense interval.

There is a discrepancy between line and area, where the default y = identity is set:

export function lineY(data, {x = indexOf, y = identity, ...options} = {}) {
  return new Line(data, maybeDenseIntervalX({...options, x, y}));
}
export function areaY(data, options) {
  const {x = indexOf, ...rest} = maybeDenseIntervalX(options);
  return new Area(data, maybeStackY(maybeIdentityY({...rest, x1: x, x2: undefined}, x === indexOf ? "y2" : "y")));
}

in line, it is set immediately, whereas in area it is set in maybeIdentityY, which comes after the determination of the dense interval in maybeDenseIntervalX. By way of consequence, the dense interval does not apply the reducer to any channel.

With this change, the default reducer is applied to y when it's undefined.

An unwelcome consequence is that this defines y even when y1/y2 are defined? But I don't think it has an impact in practice, because when y2 is used y is ignored.

closes #2328


For fun I tried to solve this with Claude Code.

I've left the agent's solution as the first commit (including unit tests); I did not touch the files at all for that first commit, so I signed it with Claude's email. (The commit message is also Claude's, although I added the costs below, because the agent told me it didn't have access to this information.)

In the second commit I reversed Claude's approach, and applied a human solution instead. Is it better or worse? I'll ask copilot for an opinion.

claude and others added 3 commits June 6, 2025 14:08
Fixed areaY to work with dense intervals (interval + reduce) without
requiring an explicit y option, matching lineY behavior.

🤖 Generated with [Claude Code](https://claude.ai/code)

Total cost:            $5.51
Total duration (API):  23m 31.3s
Total duration (wall): 14h 30m 4.3s
Total code changes:    208 lines added, 53 lines removed
Token usage by model:
    claude-3-5-haiku:  204.7k input, 4.8k output, 0 cache read, 0 cache write
       claude-sonnet:  18.9k input, 28.4k output, 8.3m cache read, 629.5k cache write
@Fil Fil requested review from mbostock and Copilot June 6, 2025 13:22
Copilot

This comment was marked as resolved.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

More description about how this fixes the problem would be helpful in the PR. (And it would be more relevant than your experience with AI, though I appreciate you sharing!) I guess this changes the behavior exactly when y is undefined, but preserves the current behavior when y is null. Will anything else be affected by this fix?

@Fil
Copy link
Contributor Author

Fil commented Jun 6, 2025

Good call. I added details in the description.

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.

The dense interval option on the area mark seems to require the y option
3 participants