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

plotIndividualTimeProfile and plotPopulationTimeProfile line type #926

Closed
PavelBal opened this issue May 11, 2022 · 8 comments
Closed

plotIndividualTimeProfile and plotPopulationTimeProfile line type #926

PavelBal opened this issue May 11, 2022 · 8 comments
Assignees

Comments

@PavelBal
Copy link
Member

Default line type should be "solid" and not "dashed".

@IndrajeetPatil
Copy link
Member

Please don't create new issues for this.

All these points should be mentioned in the Discussion.

@PavelBal
Copy link
Member Author

I want to merge the PR ASAP and continue working on improvements in separate issues. In the discussion, there is no way to track what has been fixed.

@PavelBal PavelBal reopened this May 11, 2022
@IndrajeetPatil
Copy link
Member

Discussion is not the right place to see what is implemented.

It is the example plots in the PR, which are updated after each commit:

https://github.com/Open-Systems-Pharmacology/OSPSuite-R/pull/871/files

image

IndrajeetPatil added a commit that referenced this issue May 11, 2022
@IndrajeetPatil
Copy link
Member

Fixed in 0c213b7

@msevestre
Copy link
Member

It's absolutely ok to create issue for this. The PR can reference the issue

@IndrajeetPatil
Copy link
Member

We will just have to agree to disagree then.

@msevestre
Copy link
Member

Yep. Just don't close the issue if someone takes the time to create them. Just reference it in your PR

@IndrajeetPatil
Copy link
Member

Yeah, but I am not ignoring them by closing them. I closed the issues and immediately created comments in the respective Discussion.

Apologies to @PavelBal if he felt that I ignored his comments.

Every new issue that is created with respect to the pending PR is a new thing for me to keep track of.

I am currently juggling:

  • ospsuite issues
  • Discussion in ospsuite
  • tlf issues,
  • PR comments
  • personal correspondence
  • Pavel's original proposal docs for these plots

So the narrower the surface area we keep for specifications, the more probable it is that I don't miss something.

PavelBal added a commit that referenced this issue May 11, 2022
* adds first drafts for plot config function + class

> “I became insane, with long intervals of horrible sanity.” - Edgar Allan Poe

* more aesthetics

* cover more properties

* remaining not necessary, or derivative of existing

* docs

* docs

* fixes

* remove unused parameters

* Update DESCRIPTION

* minor

* use `LabelConfiguration` for labels

* add background configuration

* add an intermediary class

this makes ospPlotConfig more general purpose class

* Update plotIndividualTimeProfile.R

* update

* remove `createPlotConfiguration()`

* use enums in docs

* choose defaults

* bump version of ospsuite.utils

* minor

* defaults for export

* remove constructor; add aesthetics for annotations

* rename the class; adapt everything for this change

also change file names for this change

* no constructor needed for internal class either

* rename file

* choosing defaults

* minor

* fix build failures

* remaining substitutions

* remove options that don't make sense

* make the default `NULL`

* remove unnecessary fields; catch up with tlf

* more parameter cleanup

* Use ospDefault color map

* remove spurious parameter

* fix alignment issues

* non grouped datasets get their own grouping

* fix color issue

* choose better axes default labels

* respect user defined color maps as well

* reduce size of axes labels; add example

* add NEWS; move tlf to Suggests

* can't re-export & have tlf in Suggests same time

cc @Yuri05

* re-export for now

* more docs; tlf as hard dep; elementary class tests

* add visual regression test

* more enum; more tests

* skip for now

visual regression tests are fragile; need to know where the subtle differences are coming from

* simplify updating legend data

* first draft of `plotPopulationTimeProfile()`

* minor cleanup

* improve population profile plot

* also anticipate only observed/simulated contexts

* anticipate missing error unit column

* support caption; try running tests on CI again

* tests for `plotIndividualTimeProfile()`

* test for `plotPopulationTimeProfile()`

* skip population test on AppVeyor

* no unit in axes labels when unitless

* use helper to remove duplicated code

* address more review comments

* not necessary anymore

* get rid of `DefaultInternalPlotConfiguration`

* separate linetype per dataset

* revert linetype change; anticipate other labels

* set default aesthetic maps on plot-by-plot basis

* closes #926

* docs

* More docs

* section for scaling

* set tlf data to NULL when dataType isn't present

* also update docs

* Update plot-population-time-profile.R

* add visual regression test for population profile

* also test that changing linetypes works

* more docs

* extract out common plotting function

Co-Authored-By: Pavel Balazki <pavel.balazki@gmail.com>

Co-authored-by: Pavel Balazki <pavel.balazki@gmail.com>
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

No branches or pull requests

3 participants