openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: Sigma: Uncertainty Propagation for C++ #7404

Open editorialbot opened 4 weeks ago

editorialbot commented 4 weeks ago

Submitting author: !--author-handle-->@jwaldrop107<!--end-author-handle-- (Jonathan Waldrop) Repository: https://github.com/QCUncertainty/sigma Branch with paper.md (empty if default branch): joss_paper Version: v0.1 Editor: !--editor-->@vissarion<!--end-editor-- Reviewers: @baxmittens, @YehorYudinIPP Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/46af69c48a681200c40fb5a9cebc6168"><img src="https://joss.theoj.org/papers/46af69c48a681200c40fb5a9cebc6168/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/46af69c48a681200c40fb5a9cebc6168/status.svg)](https://joss.theoj.org/papers/46af69c48a681200c40fb5a9cebc6168)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@baxmittens & @YehorYudinIPP, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @vissarion know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

πŸ“ Checklist for @YehorYudinIPP

πŸ“ Checklist for @baxmittens

editorialbot commented 4 weeks ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 4 weeks ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.02 s (2367.9 files/s, 179335.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             17            133             10           1041
C/C++ Header                    14            161            891            432
Markdown                         4             53              0            418
YAML                             3              7             17            187
CMake                            8             36             29            170
TeX                              1              4              0             46
HTML                             1              3             19             45
CSS                              1              1              2              6
-------------------------------------------------------------------------------
SUM:                            49            398            968           2345
-------------------------------------------------------------------------------

Commit count by author:

    86  Jonathan M. Waldrop
     1  Ryan Richard
editorialbot commented 4 weeks ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

βœ… OK DOIs

- 10.1063/5.0196384 is OK

🟑 SKIP DOIs

- No DOI given, and none found for title: Uncertainty propagation with functionally correlat...
- No DOI given, and none found for title: Uncertainties: a Python package for calculations w...
- No DOI given, and none found for title: CMake
- No DOI given, and none found for title: Eigen

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
editorialbot commented 4 weeks ago

Paper file info:

πŸ“„ Wordcount for paper.md is 1064

βœ… The paper includes a Statement of need section

editorialbot commented 4 weeks ago

License info:

βœ… License found: Apache License 2.0 (Valid open source OSI approved license)

editorialbot commented 4 weeks ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

YehorYudinIPP commented 4 weeks ago

Review checklist for @YehorYudinIPP

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

baxmittens commented 4 weeks ago

Hi all,

Nice paper and software project. Congratulations! I wanted to look into this topic for a long time. I will therefore gladly take this opportunity.

I read the paper and one particular thing that caught my attention was

""" These element are defined as π‘Žπ‘– = Μ„π‘Žπ‘– Β± 𝜎_π‘Žπ‘– , where Μ„π‘Žπ‘– is the mean value of the variable and 𝜎_π‘Žπ‘– is its standard deviation. """

I think the standard deviation is the square root of the variance of a random variable which is equipped with - or linked to - a probability distribution. Therefore I think 𝜎_π‘Žπ‘– should rather be called 'uncertainty', since it does not own a probability measure. Surely, you can say something like '𝜎_π‘Žπ‘– is called the uncertainty and is assumed to represent an error measure closely related to the standard deviation of a random variable' (or something like that). I think in one of your sources https://arxiv.org/abs/1610.08716 this is also more carefully phrased.

Next thing would be

""" The uncertainty of 𝐹 (𝐴) can be determined as 𝜎_𝐹 β‰ˆ ... """

I think this should also rather be described as the linear uncertainty (or something alike) since in general the pdf of F(A(Ο‰)) will be skewed, and, in that case, the standard deviation doesn't represent too much of the uncertainty of the outcome (see the example of a cantilever with uncertain tip displacement which I attache). In general, the difference between the method at hand and 'uncertainty quantification' which involves integration over a probability space should be briefly pointed out, I think.

Greetz Max

image
using Distributions
using ProgressMeter
import .Threads: @threads
using CairoMakie
using Measurements
using Statistics

function cantilever_displacement(E,X,Y,L,w,t)
    # see https://www.sfu.ca/~ssurjano/canti.html
    Dfact1 = 4*(L^3) / (E*w*t)
    Dfact2 = sqrt((Y/(t^2))^2 + (X/(w^2))^2)
    D = Dfact1 * Dfact2
    return D
end

N_E = Normal{Float64}(2.9e7, 1.45e6) # Young's modulus
N_X = Normal{Float64}(500.0, 50.0)  # horizontal load
N_Y = Normal{Float64}(1000.0, 100.0) # vertical load
N_L = Normal{Float64}(100.0, 10.0) # beam length
N_w = Normal{Float64}(4.0, 0.4) # beam width
N_t = Normal{Float64}(2.0, 0.2) # beam width

M_E = measurement(2.9e7, 1.45e6) # Young's modulus
M_X = measurement(500.0, 50.0)  # horizontal load
M_Y = measurement(1000.0, 100.0) # vertical load
M_L = measurement(100.0, 10.0) # beam length
M_w = measurement(4.0, 0.4) # beam width
M_t = measurement(2.0, 0.2) # beam width

samplefunc() = [rand(N_E), rand(N_X), rand(N_Y), rand(N_L), rand(N_w), rand(N_t)]

N = 5_000_000
monte_carlo_resvec = Vector{Float64}(undef, N)

@showprogress @threads for i = 1:N
    monte_carlo_resvec[i] = cantilever_displacement(samplefunc()...)
end

sample_mean = foldl(+, monte_carlo_resvec)/N
sample_var = foldl(+, map(x->(x-sample_mean)^2, monte_carlo_resvec))/(N-1)
sample_sqrt_var = sqrt(sample_var)
quantvals = map(x->cdf(Normal{Float64}(0.0,1.0),x), -3:3)
sample_quantiles = quantile(monte_carlo_resvec, quantvals)

Οƒ_F = cantilever_displacement(M_E,M_X,M_Y,M_L,M_w,M_t)

f = Figure(size=(600,300));
ax = Axis(f[1,1:4])
xlims!(ax, [0,20])
ylims!(ax, [0,.25])

band!(ax, [sample_quantiles[1], sample_quantiles[2]], 0, 5, color=(:red,0.1), label="99.7% quantile")
band!(ax, [sample_quantiles[end-1], sample_quantiles[end]], 0, 5, color=(:red,0.1))
band!(ax, [sample_quantiles[2], sample_quantiles[3]], 0, 5, color=(:yellow,0.2), label="95.4% quantile")
band!(ax, [sample_quantiles[end-2], sample_quantiles[end-1]], 0, 5, color=(:yellow,0.2))
band!(ax, [sample_quantiles[3], sample_quantiles[5]], 0, 5, color=(:green,0.2), label="68.2% quantile")
hist!(ax, monte_carlo_resvec, normalization = :pdf, bins = 500, color=(:red, 0.45), strokewidth=0.1, label="histogram")
lines!(ax, [sample_mean, sample_mean], [0.0,0.25], label="exp. value")
lines!(ax, [sample_mean+sample_sqrt_var, sample_mean+sample_sqrt_var], [0.0,0.25], label="mean+√var")
lines!(ax, [sample_mean-sample_sqrt_var, sample_mean-sample_sqrt_var], [0.0,0.25], label="mean-√var")
lines!(ax, [Οƒ_F.val, Οƒ_F.val], [0.0,0.25], label="Οƒ_F.val",linestyle=:dash)
lines!(ax, [Οƒ_F.val+Οƒ_F.err, Οƒ_F.val+Οƒ_F.err], [0.0,0.25], label="Οƒ_F.val+Οƒ_F.err",linestyle=:dash)
lines!(ax, [Οƒ_F.val-Οƒ_F.err, Οƒ_F.val-Οƒ_F.err], [0.0,0.25], label="Οƒ_F.val-Οƒ_F.err",linestyle=:dash)
Legend(f[1,5], ax, labelsize=8)

f

@jwaldrop107

Edit: Thought about it and now I think you can call 𝜎_π‘Ž a (empirical) standard deviation...but without knowledge of the pdf of a, this does not contain too much useful information, unless you assume your measurement error is normal distributed which does not necessarily needs to be true.

baxmittens commented 2 weeks ago

Review checklist for @baxmittens

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

baxmittens commented 1 week ago

Documentation

A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

There is a motivation section but it is a bit short. But a statement of need is given in the paper. This could be used as a template for the documentation.

Community guidelines: Are there clear guidelines for third parties wishing to 1. Contribute to the software 2. Report issues or problems with the software 3. Seek support

I think community guidelines are missing.

jwaldrop107 commented 1 day ago

@editorialbot generate pdf

Hi all,

Sorry about the delayed response. I've pushed some changes based on the comments made so far (wording in the paper, statement of need in the documentation, and community guidelines).

Thanks for taking the time to review this, and I'm looking forward to any additional comments/recommendations.

editorialbot commented 1 day ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: