Fixed coord_equal size #243
Conversation
| expect_equal(max_prop, 1) | ||
|
|
||
| # Both proportions should be positive and <= 1 | ||
| expect_true(all(spaces$x > 0 & spaces$x <= 1)) |
There was a problem hiding this comment.
please use expect_gt(spaces$x, 0) instead of expect_true()
etc.
|
|
||
| # Verify it compiles without error | ||
| info <- animint2dir(viz, open.browser = FALSE) | ||
| expect_true(file.exists(file.path(info$out.dir, "plot.json"))) |
There was a problem hiding this comment.
this test is not sufficient to test the desired functionality.
can you please move the test case to render-renderer-coord_equal.R and use animint2HTML() instead of animint2dir()?
please add a test case involving the size of the plotted region that fails for the current master.
There was a problem hiding this comment.
Sir please review the new PR and other PR's I have made ,
please also give your feedback on what mistake i should avoid and if any correction needed also tell , Thank You 🙏!
|
I have tested the new test failed on the OLD code (showing plots of very small size ), Before But on new test passed on the new code (the test which was causing the issue , PLot coming in very small size is now fixed ), After SUMMARY:-
|
…ssarily small plots that didn't fill the available plotting area.
ANAMASGARD
force-pushed
the
coord_equal-issue-#234
branch
from
b2968b8 to
ab19433
Compare
|
|
||
| # Changes in version 2025.10.4 (Issue #234) | ||
|
|
||
| - Fixed `coord_equal()` and `coord_fixed()` to properly fill available plotting space. Previously, plots with fixed aspect ratios were unnecessarily shrunk due to incorrect normalization in the `fixed_spaces()` function. The fix changes from using `min(z, 1)` to normalizing by the maximum value across both dimensions, ensuring at least one dimension fills the available space while maintaining the correct aspect ratio. |
| info="At least one dimension should fill available space") | ||
|
|
||
| # Both dimensions should be positive | ||
| expect_gt(svg.width, 0) |
There was a problem hiding this comment.
I'm not sure these tests are robust/simple enough.
please consider adding this test instead, to the existing file, test-renderer1-coord.R
test_that("xaxis width increases with coord_equal", {
p_equal <- p + coord_equal()
animint2HTML(animint(p_equal))
bbox_default <- get_element_bbox("g.xaxis")
animint2HTML(animint(p_equal+theme_animint(width=1000)))
bbox_big <- get_element_bbox("g.xaxis")
ratio <- bbox_big$width/bbox_default$width
expect_gt(ratio, 2)
})
| # Test that at least ONE dimension is close to requested size (500) | ||
| max_dimension <- max(svg.width, svg.height) | ||
| expect_gt(max_dimension, 450, | ||
| info="At least one dimension should fill available space") |
There was a problem hiding this comment.
this test case gives me the error below.
> expect_gt(max_dimension, 450,
+ info="At least one dimension should fill available space")
Error in expect_gt(max_dimension, 450, info = "At least one dimension should fill available space") :
unused argument (info = "At least one dimension should fill available space")but after removing the info argument, the test passes under current master, so this test case is not sufficient for issue #234
| spaces <- animint2:::fixed_spaces(ranges, ratio = 1) | ||
|
|
||
| # At least one dimension should be 1 (fills available space) | ||
| max_prop <- max(spaces$x, spaces$y) |
There was a problem hiding this comment.
this test also passes under current master so is not sufficient to test #234, please remove
There was a problem hiding this comment.
in the future, please push test cases that fail to your PR, before you push the fix.
so then we can see the red X for the new failing test case, which becomes green when you push the fix.
|
the code in this PR breaks the tests in test-renderer1-coord.R can you please investigate and push a fix to #253 ? |
2 participants
This fixes issue #234 where coord_equal() was producing unnecessarily
small plots that didn't fill the available plotting area.
Changes:
R/z_facets.R: Normalize plot proportions so the largest dimension
equals 1, allowing plots to fill available space while maintaining
the correct aspect ratio. Previously used min(z, 1) which clamped
both dimensions and caused excessive shrinking.
inst/htmljs/animint.js: Remove Math.min() from proportion calculations
that was re-shrinking plots after R-side normalization. Add proper
normalization after applying aspect ratio to ensure at least one
dimension fills the browser viewport.
tests/testthat/test-compiler-coord-equal-fix.R: Add unit tests to
verify the fix and prevent regression.
NEWS.md: Document the bug fix for users.
The fix ensures coord_equal() plots are properly sized and fill the
plotting area as expected, matching the behavior shown in the issue's
'desired output' screenshot.
Resolves #234"
