refactor: update `stats/base/dists/triangular/mgf` implementation and test tolerances #4768

anandkaranubc · 2025-01-15T18:20:27Z

Resolves #4704

Description

What is the purpose of this pull request?

This pull request:

updates the implementation of the triangular MGF in main.c, main.js, and factory.js to align with the latest Julia standards. It also updates test tolerances in test.native.js for consistency.
update fixtures for triangular MGF based on Julia implementation (using the same Julia version 1.5)

Related Issues

Does this pull request have any related issues?

This pull request:

resolves #4704

Questions

Any questions for reviewers of this pull request?

There is a helper function called phi2 which has been added to the C and JS files. I have added it as a private helper function in the same files. Should it be moved to a separate module for JS and a separate C file?

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

The PR reduces the tolerance thresholds for test.native.js from a factor of 1.0e7 to 4.0. The required tolerance should be 1.0, but only 2 test cases fail with a 1.0 factor tolerance. I tried investigating the root cause by changing the order of terms and avoiding operator fusion, but no changes were observed. I will need some advice on how to fix this.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

Read, understood, and followed the contributing guidelines.

@stdlib-js/reviewers

stdlib-bot · 2025-01-15T18:22:29Z

Coverage Report

Package	Statements	Branches	Functions	Lines
stats/base/dists/triangular/mgf	$\color{red}375/377$ $\color{green}+99.47\%$	$\color{red}35/36$ $\color{green}+97.22\%$	$\color{green}5/5$ $\color{green}+100.00\%$	$\color{red}375/377$ $\color{green}+99.47\%$

The above coverage report was generated for the changes in this PR.

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---

…mplementation --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: passed - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---

…nsistency --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---

…ulia --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: missing_dependencies - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: passed - task: run_c_examples status: passed - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: passed - task: run_c_benchmarks status: passed - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: passed - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: passed ---

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: missing_dependencies - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

anandkaranubc · 2025-01-15T21:26:48Z

Hii @kgryte,

Over to you for review. A follow up question in addition to the ones above:

To ensure 100% coverage, should I update runner.jl to include examples from all 4 cases, or should I just manually add additional test cases in a new tape function?

kgryte · 2025-01-16T02:19:12Z

@anandkaranubc Would you mind updating and rerunning the runner?

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/lib/factory.js

…ions --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---

…ions --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed ---

…ntation --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: passed ---

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: missing_dependencies - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

anandkaranubc · 2025-01-18T07:50:15Z

Can I get some feedback on this too please, @kgryte? Thanks!

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/lib/factory.js

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/lib/main.js

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/lib/factory.js

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/src/main.c

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/test/test.factory.js

kgryte · 2025-01-18T11:26:52Z

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/test/test.native.js

+			t.equal( y, expected[i], 'x: '+x[i]+', a: '+a[i]+', b: '+b[i]+', c: '+c[i]+', y: '+y+', expected: '+expected[i] );
+		} else {
+			delta = abs( y - expected[ i ] );
+			tol = 4.0 * EPS * abs( expected[ i ] );


While muuuuch closer to JS, it isn't a perfect match. Any idea why?

After re-reading your OP, have you tried recompiling but disabling certain compiler optimizations?

Update: I added CFLAGS to the Makefile to disable compiler optimizations (-O0 -g). This did impact the test results, with only one test failing after adjusting all tolerances from a factor of 4.0 to 1.0.

I'll bring this up in today's OH to clarify if this is the best approach or if there's an alternative solution. I've also added a question to the OH issue. See you soon!

Hi again @kgryte,

Here's an update on the issue: I realized that I was considering higher tolerances for all cases, but in most cases, a factor of 1.0 works just fine.

There is, however, one test that fails in this test category:

stdlib/lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/test/test.native.js

Line 88 in 0937887

tape( 'the function evaluates the MGF for `x` given a small range `b - a`', opts, function test( t ) {

I reviewed the discussion in #2298, but it seems that using CFLAGS="-ffp-contract=off" or CFLAGS="-ffp-contract=on" doesn't make a difference in this case.

I tracked down the first mismatch in the results between the JS and C implementations to the calculation of expm1:

stdlib/lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/src/main.c

Line 34 in 0937887

return ( 2.0 * ( stdlib_base_expm1( x ) - x ) ) / ( x * x );

The C implementation gives:
stdlib_base_expm1((a - c) * t) & stdlib_base_expm1((b - c) * t)

term1: -0.75243526099293723

term2: 0.51382856876570271

The JS implementation gives:
expm1((a - c) * t) & expm1((b - c) * t)

term1: -0.7524352609929372

term2: 0.5138285687657028

This doesn't seem to be the result of simple rounding errors like those seen in the other issue. As you mentioned, this is likely due to different implementations or approximations used for these functions. Any thoughts? :)

Also, for the one test that fails, a factor of 2.0 instead of 1.0 works. Added a comment in the recent commit.

Adjusting to a tolerance of 2.0 should be fine. In the test comment, you can link to this discussion.

Already done. Hope this looks good:

stdlib/lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/test/test.native.js

Line 111 in 2d4f12d

// NOTE: The tolerance here is larger than that of the JavaScript implementation due to differences in the calculation of `expm1`, which result in a divergence of results. For more details, see the discussion at: https://github.com/stdlib-js/stdlib/pull/4768#discussion_r1921047716

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: passed - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: missing_dependencies - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: passed ---

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: passed - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

--- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: passed ---

…nd use uniform instead of randu --- type: pre_commit_static_analysis_report description: Results of running static analysis checks when committing changes. report: - task: lint_filenames status: passed - task: lint_editorconfig status: passed - task: lint_markdown status: na - task: lint_package_json status: na - task: lint_repl_help status: na - task: lint_javascript_src status: na - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - task: lint_javascript_benchmarks status: passed - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: na - task: lint_c_examples status: na - task: lint_c_benchmarks status: na - task: lint_c_tests_fixtures status: na - task: lint_shell status: na - task: lint_typescript_declarations status: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: passed - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

anandkaranubc · 2025-01-22T14:54:21Z

Should the README.md also be changed with the updated formula for mgf calculation?

$$M_X(t) := \mathbb{E}\!\left[e^{tX}\right] = \begin{cases} \exp(ct) \frac{(c-a)\phi_2((a-c)t) + (b-c)\phi_2((b-c)t)}{b-a} & \text{if } a < c < b \\\ \exp(ct) \phi_2((a-c)t) & \text{if } a = c < b \\\ \exp(ct) \phi_2((b-c)t) & \text{if } a < c = b \\\ \exp(ct) & \text{if } a = c = b \end{cases}$$

$$\phi_2(x) := \begin{cases} 1, & \text{if } x = 0 \\\ \frac{2 \left( \exp(x) - 1 - x \right)}{x^2} & \text{if } x \neq 0 \end{cases}$$

I notice that this would also mean changing the svg link provided for the equation.

A side note: The Julia implementation does this case by case breakdown to avoid unnecessary computation.

kgryte · 2025-01-22T18:56:25Z

Should the README.md also be changed with the updated formula for mgf calculation?

No. The equations in READMEs should not necessarily reflect implementation logic. That is an implementation detail. Unless the package explicitly concerns a particular algorithm, the README contains equations as you'd find in a textbook or Wikipedia. The MGF currently in the README follows the latter.

stdlib-bot added the Needs Review A pull request which needs code review. label Jan 15, 2025

anandkaranubc marked this pull request as draft January 15, 2025 18:26

stdlib-bot removed the Needs Review A pull request which needs code review. label Jan 15, 2025

anandkaranubc added 4 commits January 15, 2025 13:11

anandkaranubc force-pushed the fix/triangular-mgf-divergence branch from 3225361 to 263cdaf Compare January 15, 2025 21:12

stdlib-bot added the Statistics Issue or pull request related to statistical functionality. label Jan 15, 2025

anandkaranubc marked this pull request as ready for review January 15, 2025 21:20

stdlib-bot added the Needs Review A pull request which needs code review. label Jan 15, 2025

kgryte reviewed Jan 16, 2025

View reviewed changes

lib/node_modules/@stdlib/stats/base/dists/triangular/mgf/lib/factory.js Outdated Show resolved Hide resolved

anandkaranubc added 5 commits January 15, 2025 21:55

kgryte changed the title ~~feat: update stats/base/dists/triangular/mgf implementation and test tolerances for Julia alignment~~ refactor: update stats/base/dists/triangular/mgf implementation and test tolerances Jan 18, 2025