Skip to content

Support interval+reduce in the area mark#2329

Merged
mbostock merged 13 commits intomainfrom
claude/2328
Apr 3, 2026
Merged

Support interval+reduce in the area mark#2329
mbostock merged 13 commits intomainfrom
claude/2328

Conversation

@Fil
Copy link
Copy Markdown
Contributor

@Fil Fil commented Jun 6, 2025

Defaults for x, y, indexOf, and identity are now set inside maybeDenseIntervalX / maybeDenseIntervalY, instead of being scattered across the mark constructors. This fixes a bug where areaY (and areaX) with a dense interval did not apply the reducer to the y (or x) channel, because the default was set too late — after the dense interval transform had already been configured.

For example, this now works as expected:

Plot.areaY(data, {x: "date", interval: "day", reduce: "count"})

Previously, the dense interval would not reduce y because y was still undefined when maybeDenseIntervalX ran. The identity default was only applied later by maybeIdentityY.

Closes #2328

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 Copilot and mbostock June 6, 2025 13:22

This comment was marked as resolved.

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Fil commented Jun 6, 2025

Good call. I added details in the description.

Copy link
Copy Markdown
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.

I figured out how to lift the defaults for dense intervals up so they are shared between line and area. That feels a little cleaner to me.

Copy link
Copy Markdown
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.

I’m less sure about this, but at least according to the tests, the area mark also doesn’t need the maybeIdentity transform now?

@Fil
Copy link
Copy Markdown
Contributor Author

Fil commented Apr 3, 2026

Merging main was a bit complicated by 3542325; then I saw that lineX could be simplified to follow the same pattern as we had in areaX (we were setting the defaults x = indexOf, y = identity in two places). PTAL?

@Fil Fil requested a review from mbostock April 3, 2026 15:12
@mbostock mbostock merged commit e31a7ae into main Apr 3, 2026
1 check passed
@mbostock mbostock deleted the claude/2328 branch April 3, 2026 17:37
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

4 participants