Skip to content
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

Fixes #333 set cap width of errorbars #344

Merged
merged 4 commits into from
Jul 25, 2022

Conversation

pchelle
Copy link
Collaborator

@pchelle pchelle commented Jul 22, 2022

Currently, width is a global setting applied the same way to all error bars

pchelle and others added 2 commits July 22, 2022 12:17
Currently, width is a global setting applied the same way to all error bars
@IndrajeetPatil
Copy link
Member

Instead of having this as a global setting, is it possible to make this a field of tlf::ThemeAestheticSelections?

This way, I can do something like the following:

  errorbarsConfiguration <- tlf::ThemeAestheticSelections$new(
    size = generalPlotConfiguration$errorbarsSize,
    linetype = generalPlotConfiguration$errorbarsLinetype,
    alpha = generalPlotConfiguration$errorbarsAlpha,
    width = generalPlotConfiguration$errorbarsCapWidth
  )

specificPlotConfiguration$errorbars <- errorbarsConfiguration

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 23, 2022

It is possible, but it will require more time to make the ThemeAestheticSelection dependent on the type of property as cap width is not applicable to ribbons, lines and scatter points.
In the end, this might be for the best since ribbons also behave quite differently (fill property not applicable to any other selection).
It would be cool to link the cap width to the json Theme as well.

@IndrajeetPatil
Copy link
Member

Okay, in order to not block this PR, I've created a new issue to track this other feature:
#347

@IndrajeetPatil
Copy link
Member

I can't get this to work.

In Open-Systems-Pharmacology/OSPSuite-R#1038, I am using the new setDefaultErrorbarCapWidth() function to set cap width, but it doesn't affect the plots. What am I doing wrong?

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 24, 2022

@IndrajeetPatil
Could you try with a different value ?
When testing ggplot2::geom_errorbar, it seemed that the width argument corresponds to the values of the x-axis and not a size in pts.
In your example, the x-axis ranges from 0 to 250, a width of about 5 or 10 should be visible.

@IndrajeetPatil
Copy link
Member

I don't think this has anything to do with the range of the data values.

In these plots, the data values are orders of magnitude different, and yet error bar width specification of 0.2 always produces the same width for caps.

library(tidyverse)

df <- data.frame(
  trt = factor(c(1, 1, 2, 2)),
  resp = c(1, 5, 3, 4),
  group = factor(c(1, 2, 1, 2)),
  upper = c(1.1, 5.3, 3.3, 4.2),
  lower = c(0.8, 4.6, 2.4, 3.6)
)

ggplot(df, aes(trt, resp, colour = group)) + 
  geom_errorbar(aes(ymin = lower, ymax = upper), width = 0.2)

df2 <- df %>%
  mutate(resp = 1e5 * resp, upper = 1e5 * upper, lower = 1e5 * lower)

ggplot(df2, aes(trt, resp, colour = group)) + 
  geom_errorbar(aes(ymin = lower, ymax = upper), width = 0.2)

Created on 2022-07-25 by the reprex package (v2.0.1)

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 25, 2022

It does but on the values of the X axis:
If I use a similar example with x being numeric instead of factor.
You can see the cap width is 0.2 when measuring with the ticks

library(tidyverse)

df <- data.frame(
  trt = c(1, 1, 2, 2),
  resp = c(1, 5, 3, 4),
  group = factor(c(1, 2, 1, 2)),
  upper = c(1.1, 5.3, 3.3, 4.2),
  lower = c(0.8, 4.6, 2.4, 3.6)
)

ggplot(df, aes(trt, resp, colour = group)) + 
  geom_errorbar(aes(ymin = lower, ymax = upper), width = 0.2)

image

Then, if I increase the x range

df <- data.frame(
  trt = c(1, 1, 10, 10),
  resp = c(1, 5, 3, 4),
  group = factor(c(1, 2, 1, 2)),
  upper = c(1.1, 5.3, 3.3, 4.2),
  lower = c(0.8, 4.6, 2.4, 3.6)
)

ggplot(df, aes(trt, resp, colour = group)) + 
  geom_errorbar(aes(ymin = lower, ymax = upper), width = 0.2)

image

df <- data.frame(
  trt = c(1, 1, 10, 10),
  resp = c(1, 5, 3, 4),
  group = factor(c(1, 2, 1, 2)),
  upper = c(1.1, 5.3, 3.3, 4.2),
  lower = c(0.8, 4.6, 2.4, 3.6)
)

ggplot(df, aes(trt, resp, colour = group)) + 
  geom_errorbar(aes(ymin = lower, ymax = upper), width = 2)

image

@IndrajeetPatil
Copy link
Member

Thanks for the clarification, Pierre!

Hmm, this is indeed a sticky problem then, since we definitely don't want users to compute the exact width of cap needed depending on the range of data values. Typically, users produce such plots in a loop, which means that the cap width argument will have to vary from dataset-to-dataset (i.e., for each iteration).

@Yuri05 and @PavelBal:

Honestly, given these complications, I'd much rather just leave the caps out of the plot.

Instead of ospsuite plots adapting to PK-Sim plots, I think the PK-Sim plots should adapt to ospsuite plots, and remove the error bar caps. They just add clutter and do not encode any extra information.

@IndrajeetPatil
Copy link
Member

Another option is to create a helper function in tlf that implements some kind of heuristic that creates cap width depending on the range of data values. And then ospsuite can use this helper internally.

But this is added complexity, and we really want to be sure these caps are worth the added complexity.

@msevestre
Copy link
Member

Where did this feature request come from?

@IndrajeetPatil
Copy link
Member

@msevestre
Copy link
Member

Ok thanks. At any rate, the user should not have to specify a width for the caps. This is purely some internal artefacts IMO

@PavelBal
Copy link
Member

@IndrajeetPatil
Copy link
Member

This cannot be something extraordinary.

It is not. Both my and Pierre's comments show how easy it is to add caps to error bars.

The issue is not creating them for a single context, the issue is finding a general solution that will produce the same error bar cap width no matter the range of the data.

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 25, 2022

I may have a better solution:
As suggested by @IndrajeetPatil , we can internally calculate a value that would adapt to the dataset (e.g. mean space between all the different x values) and provide cap width as a ratio of that value.

If I reuse the example above with tlf and this solution, I get:

addErrorbar(
x = c(1, 1, 2, 2),
ymin = c(0.8, 4.6, 2.4, 3.6),
ymax = c(1.1, 5.3, 3.3, 4.2),
capWidth = 1
)

image

capWidth of 1 would mean the cap fill all the way until the next data

addErrorbar(
x = c(1, 1, 10, 10),
ymin = c(0.8, 4.6, 2.4, 3.6),
ymax = c(1.1, 5.3, 3.3, 4.2),
capWidth = 1
)

image

addErrorbar(
x = c(1, 1, 10, 10),
ymin = c(0.8, 4.6, 2.4, 3.6),
ymax = c(1.1, 5.3, 3.3, 4.2),
capWidth = 0.2
)

image

Thoughts ?

@IndrajeetPatil
Copy link
Member

IndrajeetPatil commented Jul 25, 2022

That looks great, @pchelle! Thanks for working on this so quickly.

One remark: We should rename the parameter from capWidth to something like capExtent.

This is because, otherwise:

  • when the error bar is horizontal, the user will have to specify height
  • when the error bar is vertical, the user will have to specify width

@msevestre
Copy link
Member

when the error bar is horizontal, the user will have to specify height
when the error bar is vertical, the user will have to specify width

I don't get this. Isn't it something that the user should not have to care about?

And sorry for the stupid questions: But why would we ever want to have something like this in the context of our suite?
image

@Yuri05 Have we ever associated an actual meaning to the width of those caps? (or heights). Or is this purely for visual inspections?

@IndrajeetPatil
Copy link
Member

IndrajeetPatil commented Jul 25, 2022

Isn't it something that the user should not have to care about?

Exactly. But for that to happen, the parameter should be renamed to cover both horizontal and vertical error bars.

That's why I am suggesting naming it capExtent.

@PavelBal
Copy link
Member

Have we ever associated an actual meaning to the width of those caps? (or heights). Or is this purely for visual inspections?

The width has no meaning. It would be best if the width/height of the caps corresponds to the width/height of the symbols.

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 25, 2022

The width has no meaning. It would be best if the width/height of the caps corresponds to the width/height of the symbols.

That is the issue with ggplot2, the function adding caps is geom_errobar that use an argument based on the data values for displaying the cap extent while it uses an external measure for the symbol size (in pts).

Renaming width to extent to prevent user confusion when used for horizontal errorbars whose caps become vertical
@Yuri05
Copy link
Member

Yuri05 commented Jul 25, 2022

The width has no meaning. It would be best if the width/height of the caps corresponds to the width/height of the symbols.

Either this or fixed width. IT does not matter if I have 1 or 100 observed data points in the plot - the width (or height :)) should remain the same.

@codecov-commenter
Copy link

Codecov Report

Merging #344 (a839d0f) into develop (cdcdd20) will increase coverage by 0.03%.
The diff coverage is 61.36%.

@@             Coverage Diff             @@
##           develop     #344      +/-   ##
===========================================
+ Coverage    64.21%   64.24%   +0.03%     
===========================================
  Files           50       50              
  Lines         2979     3032      +53     
===========================================
+ Hits          1913     1948      +35     
- Misses        1066     1084      +18     
Impacted Files Coverage Δ
R/plot-timeprofile.R 25.22% <0.00%> (-3.65%) ⬇️
R/tlf-env.R 0.00% <0.00%> (ø)
R/atom-plots.R 82.24% <95.65%> (+5.21%) ⬆️
R/aaa-utilities.R 97.34% <100.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdcdd20...a839d0f. Read the comment docs.

@IndrajeetPatil
Copy link
Member

@pchelle I don't think this is working as expected as yet. You can see the plots with the current state of the PR here.

In particular, the height of the cap seems to vary widely depending on the data and the scaling used. Here are two examples:

image

image

@Yuri05
Copy link
Member

Yuri05 commented Jul 25, 2022

sometimes I really wonder why it's so hard to implement the simplest things with ggplot...

Can we set the caps width to capExtent/Number_of_plotted_observed_data_points ? Then it would always have the same fixed size.

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 25, 2022

sometimes I really wonder why it's so hard to implement the simplest things with ggplot...

I could not agree more !

@pchelle
Copy link
Collaborator Author

pchelle commented Jul 25, 2022

I have finally found a cool solution for this: not using geom_errorbar at all.
Instead, I will only add the caps as symbols ("_" for vertical errorbars and "|" for horizontal) in geom_point.
So the cap size will correspond to the size of the character in pts (consistent with arguments for lines and symbol sizes)

@Yuri05
Copy link
Member

Yuri05 commented Jul 25, 2022

I have finally found a cool solution for this: not using geom_errorbar at all. Instead, I will only add the caps as symbols ("_" for vertical errorbars and "|" for horizontal) in geom_point. So the cap size will correspond to the size of the character in pts (consistent with arguments for lines and symbol sizes)

sounds good!

Copy link
Member

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

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

Works great now. Thanks!

@IndrajeetPatil IndrajeetPatil merged commit 9640f54 into Open-Systems-Pharmacology:develop Jul 25, 2022
Yuri05 pushed a commit to Yuri05/TLF-Library that referenced this pull request Jan 27, 2023
…Systems-Pharmacology#344)

* Fixes Open-Systems-Pharmacology#333 set cap width of errorbars

Currently, width is a global setting applied the same way to all error bars

* Absolute cap values are now relative and width renamed to extent

Renaming width to extent to prevent user confusion when used for horizontal errorbars whose caps become vertical

* Fixes Open-Systems-Pharmacology#333 cap extent renamed cap size and use consistent unit in pts

Co-authored-by: Indrajeet Patil <[email protected]>
msevestre added a commit that referenced this pull request Jan 27, 2023
* Version 1.4.89 (#323)

* Fixes #280 background elements defined a priori (#284)

This allows to switch off grid at creation of plot configuration

* Fixes #281 Fixes #282 update vignettes (#290)

* Fixes #289 update news (#291)

* 273 na rm (#278)

* Fixes #273 NA are removed without producing warnings

* Fixes #272 set transparency for molecular plots

* Update line layer in scatter plots

* Fix typo

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #288 create sections in reference page of doc website (#292)

* Fixes #288 create sections in reference page of doc website

* Remove duplicated last reference section

* Export configuration output folder (#287)

* Add `path` property to `ExportConfiguration`

* Added argument `path` to `ExportConfiguration`
Documentation generated with roxygen2

* Run styler

* Fix wrong assignment of `path` property

* adopt same gitattributes as other R repos in OSP (#295)

* whitespace changes due to new gitattributes

* Improvements to the `PlotGridConfiguration` class (#293)

* Bump package version; update NEWS

* Also update website for new package version

* rest

* Update appveyor.yml

* Add print method for plot grid config

Closes #274

* document

* better example

* better headers

* add more aesthetic parameters for plot grid config

- Add needed enums
- Update NEWS for the same
- Update docs
- Add test
- Update examples

* style

* caption position

* bump version

* Add PlotAnnotationTextSize

* move enums to plot grid file

* docs

* Update PlotGridConfiguration.Rd

* needed for vdiffr tests

* move all enums to their own file

* also add watermark size to the enum (#297)

* whitespace changes due to new gitattributes

* Use utilities from ospsuite.utils (#299)

To be considered once Open-Systems-Pharmacology/OSPSuite.RUtils#115 is merged.

* Add `collate` directive (#296)

* Fixes #301 Helper function to get lines from fold distance values (#302)

* Fixes #301 Helper function to get lines from fold distance values

* Update news, doc and style

* Update NEWS.md

* Update NEWS.md

Co-authored-by: Indrajeet Patil <[email protected]>

* 262 res vs pred (#270)

* Fixes #262 update documentation of plotResVsPred

* Fixes #262 res vs time functions are aliases of res vs pred functions

Include plotResVsTime, ResVsTimePlotConfiguration and ResVsTimeDataMapping

* res vs time classes derive from res vs pred

As a consequence, they are not equal and can have different theme properties

* re-document

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #304 add enum helper for ticklabels (#305)

* Fixes #304 add enum helper for ticklabels

* Fix typo in Pi label mapping

* Fix typo in example

* If ticklabels is of type expression, numeric or function use it as is

If expression or function isIncluded throw an error so the assertion is necessary before isIncluded

* style and document

Co-authored-by: Indrajeet Patil <[email protected]>

* Prepend internal function names with a dot (#307)

As per coding guidelines.

Update docs for these functions as well.

Closes #306

* 308 fold distance (#311)

* Fixes #313 Remove badges from Website-docu, update news (#314)

Co-authored-by: Yuri05 <[email protected]>

* Fixes #260 legend titles are updated and can be defined in themes (#315)

* Fixes #260 legend titles are updated and can be defined in themes

* Remove legend and legend title reset in boxplots

* Fixes #312 remove infinite values from plots (#316)

* fixes #318 Update website (#319)

Co-authored-by: Yuri05 <[email protected]>

* Fixes #320 Website: some images not displayed (#321)

Co-authored-by: Yuri05 <[email protected]>

* 320 attempt2 (#322)

* delete old docs folder

* Fixes #320 Website: some images not displayed

Co-authored-by: Yuri05 <[email protected]>

Co-authored-by: Pierre Chelle <[email protected]>
Co-authored-by: Indrajeet Patil <[email protected]>
Co-authored-by: Pavel Balazki <[email protected]>
Co-authored-by: Yuri05 <[email protected]>

* Improvements to the `PlotGridConfiguration` class (#293)

* Bump package version; update NEWS

* Also update website for new package version

* rest

* Update appveyor.yml

* Add print method for plot grid config

Closes #274

* document

* better example

* better headers

* add more aesthetic parameters for plot grid config

- Add needed enums
- Update NEWS for the same
- Update docs
- Add test
- Update examples

* style

* caption position

* bump version

* Add PlotAnnotationTextSize

* move enums to plot grid file

* docs

* Update PlotGridConfiguration.Rd

* needed for vdiffr tests

* move all enums to their own file

* also add watermark size to the enum (#297)

* Fixes #301 Helper function to get lines from fold distance values (#302)

* Fixes #301 Helper function to get lines from fold distance values

* Update news, doc and style

* Update NEWS.md

* Update NEWS.md

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #304 add enum helper for ticklabels (#305)

* Fixes #304 add enum helper for ticklabels

* Fix typo in Pi label mapping

* Fix typo in example

* If ticklabels is of type expression, numeric or function use it as is

If expression or function isIncluded throw an error so the assertion is necessary before isIncluded

* style and document

Co-authored-by: Indrajeet Patil <[email protected]>

* 308 fold distance (#311)

* 320 attempt2 (#322)

* delete old docs folder

* Fixes #320 Website: some images not displayed

Co-authored-by: Yuri05 <[email protected]>

* Fixes #325 legend title properties from argument title are used (#328)

Issue was caused because properties of R6 (Label object here) are linked if object is not cloned

* Fixes #327 horizontal bars for obs vs pred plots (#331)

- error is replaced in favor of ymin/ymax or xmin/xmax
- dataMapping initialize method needs to explicitly redefine `x` and `y` arguments because of R partial matching (if user inputs `x="a"`, partial matching would assign "a" to `xmin` instead of `x` because `xmin` was the only argument explicitly defined)

* Refresh README (#335)

* Refresh README

Closes #153
Closes #334

* remove README.Rmd

* Update README.md

* Classify vignettes on the website (#336)

Closes #178

* Make `.Rbuildignore` more comprehensive (#337)

* Make `.Rbuildignore` more comprehensive

Get rid of `NOTE` about unexpected top-level files

* Update .Rbuildignore

* Update to roxygen2 7.2.1 (#339)

* Format with latest version of styler (#341)

* Fixes #326 legend title use plot configuration for time profile plots (#343)

* Get rid of warnings and notes in R CMD check (#338)

* Fixes #333 set cap width of errorbars (#344)

* Fixes #333 set cap width of errorbars

Currently, width is a global setting applied the same way to all error bars

* Absolute cap values are now relative and width renamed to extent

Renaming width to extent to prevent user confusion when used for horizontal errorbars whose caps become vertical

* Fixes #333 cap extent renamed cap size and use consistent unit in pts

Co-authored-by: Indrajeet Patil <[email protected]>

* 303 obs map to shape (#351)

* Fixes #303 observed data map to shape

* Fixes #346 remove dynamic code

* Update documentation with latest roxygen

* format with styler

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #353 time profile legend guide display and order shapes (#355)

* Fixes #356 display minor ticks (#357)

* Fixes #356 display minor ticks

* fix test

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #354 ObservedDataMapping signature consistent with TimeProdfle (#358)

* Fixes #360 add linetype as internal splitting group for simulation range (#361)

* Fixes #350 default obs vs pred plot use same axes limits (#365)

The same option also centers residuals vs pred/time around 0

* format with {styler} (#366)

* Fixes #368 Add identity in Scaling enum (#371)

Also fix a typo in enum Shapes

* Fixes #375 Add spellcheck

* Fixes #362 introduce qq plots (#370)

* Fixes #362 introduce qq plots

* Remove empty y check overkill

Co-authored-by: Indrajeet Patil <[email protected]>

* Fixes #364 introduce cumulative time profile plots (#378)

* 374 enum listing molecules (#380)

* Fixes #375 enums listing all molecules, atoms and their configurations

* Fixes #379 Synchronize theme and molecule plots

* Rename Atoms and Molecules to  AtomPlots and MoleculePlots

* Fixes #381 Prevent log ticks from crashing plots (#382)

* Fixes #387 enforce factor type to prevent crash of colorBreaks (#388)

levels of character type variable is null possibly leading to null colorBreaks value

* Fixes #383 histogram can plot bars as frequency (#384)

* Fixes #383 histogram can plot bars as frequency

* Update documentation about default values of histogram

* Fixes #390 Fixes #391 observed and simulated time profiles (#393)

* Fixes #392 Create method for dual axis time profile plots (#396)

* Fixes #392 Create method for dual axis time profile plots

* Update test plot grid snapshot to latest

* Fixes #398 update documentation and website (#399)

* Fixes #398 update documentation and website

* Fixes #398 Update version and dev documentation

* Update release documentation

* Fixes #397 vignette code is run only if R version is >= 4.0 (#401)

* Increment AppVeyour version (1.4=>1.5) (#402)

Co-authored-by: Yuri05 <[email protected]>

* merge main into develop

---------

Co-authored-by: Michael Sevestre <[email protected]>
Co-authored-by: Pierre Chelle <[email protected]>
Co-authored-by: Indrajeet Patil <[email protected]>
Co-authored-by: Pavel Balazki <[email protected]>
Co-authored-by: Yuri05 <[email protected]>
@pchelle pchelle deleted the 333_caps branch February 15, 2024 16:19
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.

6 participants