joaopedroab commented on PR #38458:
URL: https://github.com/apache/superset/pull/38458#issuecomment-4056413422
@richardfogaca
Thanks for raising this. I reviewed the issue and the final branch behavior
again, and the current implementation is intentional.
The original problem here was not only the `NaN` case for negative values,
but also preserving visible and differentiated cluster sizing once
negative-capable aggregations are present. Clamping all negatives to the floor
avoided the crash, but it collapsed all-negative datasets into the same
minimum radius, which was the gap called out in review.
The branch now makes that behavior explicit:
- bubble label keeps the signed aggregated value
- bubble radius is scaled by the absolute magnitude of that value
That means `-100` renders larger than `-1` because the visual encoding is
magnitude-based, not signed-order-based. I agree that this would be ambiguous
if it were accidental, so I tightened the regression coverage to pin
that behavior explicitly across `sum`, `min`, `max`, and `mean`, including
all-negative and mixed-sign cases.
Given the issue this PR is solving, I think magnitude-based sizing is the
better choice for this change set. If we want bubble size to represent signed
ordering instead, that would be a separate semantic/product decision
rather than a follow-up bug fix inside this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
