Skip to content

Conversation

@SteveBronder
Copy link
Contributor

@SteveBronder SteveBronder commented Jul 26, 2024

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here: Still needs added
    • OR, no user-facing changes were made

Release notes

Adds row and column stochastic matrix types

stochastic_row_matrix[10, 10] X;
stochastic_column_matrix[10, 10] X;

See the example here for more

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a few scattered questions

@codecov
Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.52%. Comparing base (4f0d26c) to head (4247ad4).

Files Patch % Lines
src/frontend/Ast_to_Mir.ml 76.92% 3 Missing ⚠️
src/frontend/Pretty_printing.ml 50.00% 2 Missing ⚠️
src/middle/Transformation.ml 0.00% 2 Missing ⚠️
src/analysis_and_optimization/Mir_utils.ml 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1442      +/-   ##
==========================================
- Coverage   89.56%   89.52%   -0.05%     
==========================================
  Files          63       63              
  Lines       10593    10614      +21     
==========================================
+ Hits         9488     9502      +14     
- Misses       1105     1112       +7     
Files Coverage Δ
src/frontend/Typechecker.ml 91.77% <100.00%> (+0.01%) ⬆️
src/stan_math_backend/Lower_expr.ml 93.54% <100.00%> (+0.03%) ⬆️
src/analysis_and_optimization/Mir_utils.ml 76.40% <0.00%> (-0.36%) ⬇️
src/frontend/Pretty_printing.ml 92.25% <50.00%> (-0.61%) ⬇️
src/middle/Transformation.ml 91.30% <0.00%> (-8.70%) ⬇️
src/frontend/Ast_to_Mir.ml 94.34% <76.92%> (-0.28%) ⬇️

Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

One testing comment and one optional item but otherwise this looks good to me

@bob-carpenter
Copy link
Member

On further reflection, I think the reason we use "simplex" and "corr_matrix" is that's how someone would say them in conversation. Nobody says "simplex vector" but everyone says "correlation matrix". We weren't consistent with "ordered", which someone would say as "ordered vector".

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