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

888 residuals #939

Merged
merged 14 commits into from
Oct 19, 2022
Merged

Conversation

pchelle
Copy link
Collaborator

@pchelle pchelle commented Oct 13, 2022

No description provided.

@Yuri05
Copy link
Member

Yuri05 commented Oct 13, 2022

some tests failed on all systems...

NAMESPACE Outdated
@@ -1,5 +1,7 @@
# Generated by roxygen2: do not edit by hand

export("geomean*sd")
export("geomean/sd")
Copy link
Member

Choose a reason for hiding this comment

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

Is this a valid function name

Copy link
Member

Choose a reason for hiding this comment

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

@pchelle Please rename the functions. You have to call the functions in quotes otherwise

`geomean*sd`(c(1,2,3))

Not sure if export must contain the quotes as well then, but having such a function name is pretty crazy from my point of view.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I don't even know why this is allowed

@codecov-commenter
Copy link

Codecov Report

Base: 72.59% // Head: 70.47% // Decreases project coverage by -2.12% ⚠️

Coverage data is based on head (fe89ca9) compared to base (6000a4e).
Patch coverage: 50.43% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #939      +/-   ##
===========================================
- Coverage    72.59%   70.47%   -2.13%     
===========================================
  Files           58       58              
  Lines         7379     7719     +340     
===========================================
+ Hits          5357     5440      +83     
- Misses        2022     2279     +257     
Impacted Files Coverage Δ
R/messages.R 77.77% <ø> (ø)
R/reportingengine-env.R 41.66% <0.00%> (-50.44%) ⬇️
R/gof-plot-task.R 23.76% <4.41%> (-71.92%) ⬇️
R/utils.R 63.47% <62.50%> (-0.11%) ⬇️
R/task-settings.R 70.00% <93.33%> (+4.11%) ⬆️
R/captions.R 73.43% <94.44%> (+6.77%) ⬆️
R/utilities-goodness-of-fit.R 90.95% <98.79%> (+1.96%) ⬆️
R/qualification-gof.R 96.38% <100.00%> (+0.02%) ⬆️
R/utilities-logging.R 79.72% <100.00%> (+1.06%) ⬆️
R/utilities-observed-data.R 95.53% <100.00%> (+9.90%) ⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ymax = tlf::tlfStatFunctions$`Percentile97.5%`,
yCaption = "median",
# The unicode characters below are superscript th
rangeCaption = "[2.5\u1d57\u02b0-97.5\u1d57\u02b0] percentiles"
Copy link
Member

Choose a reason for hiding this comment

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

when entering this in R I get the following result:

> "[2.5\u1d57\u02b0-97.5\u1d57\u02b0] percentiles"
[1] "[2.5\u1d57ʰ-97.5\u1d57ʰ] percentiles"

But the unicode seems to be correct.
Does it look good when creating a report?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On my local computer it does.
I checked the default printing documentation that indicates the following:
Whether a character is printable depends on the current locale and the operating system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And when checking using vscode on the md document, it also does look good.
My plots also looks okay, but I don't know if they will on your side

Copy link
Member

Choose a reason for hiding this comment

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

interesting...
OK, let's keep it for now AS IS, I will check later how the reports look like e.g. on my machine

ymax = "geomeanMultipliedBySD",
yCaption = "geometric mean",
# The unicode character below is supposed to be */ symbol
rangeCaption = "mean \u22c7 geometric SD range"
Copy link
Member

Choose a reason for hiding this comment

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

was the caption supposed to be "mean */ geometric SD range"?
because when entering this in R I get:

> "mean \u22c7 geometric SD range"
[1] "mean ⋇ geometric SD range"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I checked for the unicode of +/- and found there was also one for */
If it looks weird I can revert and use */ instead

Copy link
Member

@Yuri05 Yuri05 Oct 19, 2022

Choose a reason for hiding this comment

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

Let's use */.
After looking at the symbol in very big resolution (https://www.compart.com/en/unicode/U+22c7) I finally understood what it is, but in the smaller size it just looks like "*"

Comment on lines +594 to +596
simulatedData <- removeNegativeValues(simulatedData, dataMapping$y)
observedData <- removeNegativeValues(observedData, dataMapping$y)
lloqData <- removeNegativeValues(lloqData, dataMapping$y)
Copy link
Member

Choose a reason for hiding this comment

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

There are some problems with this solution.
But I would suggest to address them in the separate PR in order not to block this PR for even longer.
(just created an issue for this: #942)

I have already created the issue for this in the ospuite-R (because residuals are calculated there as well):
Open-Systems-Pharmacology/OSPSuite-R#1091

In short:

  • Removing <=0 values (especially observed values) is not correct. E.g. if we have observed = 0 and simulated >>0, we just ignore a significant residual in such a case.
  • Very small positive values are problematic for log residuals as well. E.g. having observed = 1e-100 and simulated = 1e-200 would produce a huge log residual, but in fact the difference is irrelevant - both values are numerical zero.

For this, PK-Sim/MoBi introduces the LogSafe function, which is used for calculating log residuals and defined as:
LogSafe(Y) = Log(Max(Y, LogSafeEpsilon)) (with LogSafeEpsilon currently defined as 1E-20)

And Lin residuals are calculated with all the values AS IS

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.

4 participants