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

Implement concrete Error types for each distribution's new function #265

Merged

Conversation

FreezyLemon
Copy link
Contributor

@FreezyLemon FreezyLemon commented Aug 16, 2024

See #221 (comment).

Ignore the massive amount of commits, that's for my organisation and can/will be squashed later.

Outstanding issues:

  • Clippy (Empirical): Returning a Result<_, ()> is not best practice. clippy suggests an Option which makes less sense here, so either ignore or return a Empirical directly (the function is infallible) ignored

  • MultinomialError has not been implemented (yet). I noticed that the parameter validation is moved to the internal module which implies the check_multinomial function will be reused somewhere else, so I didn't want to start changing stuff before that project is done Changed my mind on this. Should be fine to change, it's not much code to replicate inside new (e.g. Categorical does basically the same check inside new)

  • Unit Tests

  • Proofread

  • After everything is done: Check StatsError to see where it is still used and if it can be replaced by something else

@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch 3 times, most recently from 93b5c2c to ece3dac Compare August 16, 2024 21:28
@FreezyLemon FreezyLemon changed the title Implement concrete Error types each distribution's new function Implement concrete Error types for each distribution's new function Aug 16, 2024
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch 2 times, most recently from 1870451 to 5cc30d2 Compare August 16, 2024 21:58
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from 5cc30d2 to 04bd36d Compare September 4, 2024 12:58
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 98.91540% with 5 lines in your changes missing coverage. Please review.

Project coverage is 93.58%. Comparing base (d0a5b04) to head (6cd01db).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/stats_tests/fisher.rs 62.50% 3 Missing ⚠️
src/distribution/multivariate_normal.rs 95.23% 1 Missing ⚠️
src/distribution/multivariate_students_t.rs 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   93.54%   93.58%   +0.04%     
==========================================
  Files          53       53              
  Lines       11657    11822     +165     
==========================================
+ Hits        10904    11064     +160     
- Misses        753      758       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FreezyLemon
Copy link
Contributor Author

#269 would reduce the effort needed to add tests in this PR significantly, so I'll wait for that.

@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from 04bd36d to aa543c4 Compare September 4, 2024 19:51
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from aa543c4 to 1e62fa7 Compare September 5, 2024 20:35
@FreezyLemon
Copy link
Contributor Author

FreezyLemon commented Sep 5, 2024

  • rebased
  • added a new test function test_create_err which expects a specific error variant
  • added a lot of tests
  • added FishersExactTestError (only one variant atm, which wraps around Hypergeometric -> bit boilerplate-y, but most flexible)

edit: Coverage is still only so-so because there is no code path hitting the Display implementations. I think we'll have to take that hit, writing unit tests for that is too much IMO

@YeungOnion
Copy link
Contributor

Regarding coverage, our CI is running nightly cargo, so we could use this attribute on functions (from cargo-llvm-cov)

in lib.rs

/// ... docs

#![cfg_attr(coverage_nightly, feature(coverage_attribute))]

in modules

impl std::fmt::Display for D {
    #[cfg_attr(coverage_nightly, coverage(off))]
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        //...
    }
}

in manifest

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage,coverage_nightly)'] }

Let me know if you want me to do the proofreading as part of my review and I'll check that off when I've done it.

@YeungOnion YeungOnion mentioned this pull request Sep 6, 2024
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch 2 times, most recently from 2208742 to 1d966b1 Compare September 6, 2024 07:57
@FreezyLemon
Copy link
Contributor Author

Let me know if you want me to do the proofreading as part of my review and I'll check that off when I've done it.

Thanks for offering. I plan on going through the docs and Display impls once myself, there's some inconsistencies that I know of which I want to fix.

Feel free to proofread when reviewing, but I'd wait until I've done my first pass.

@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from 1d966b1 to 8d02292 Compare September 6, 2024 08:05
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from 8d02292 to ffc095a Compare September 8, 2024 08:29
Includes notable changes:
- Add internal `test_create_err` function
- Use `Beta` in unit tests for testing_boiler
- Validate Dirichlet params inside `new` and
  remove `is_valid_alpha` function
- Use `Result<Empirical, ()>` for Empirical
  (infallible `::new` function)
- Validate Multinomial params inside `new`
  and remove `check_multinomial` function
- Add a concrete error type to fisher's exact
  test, too (it is dependent on Hypergeometric,
  which is why it's included in this change)
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from ffc095a to f8793d1 Compare September 8, 2024 08:35
@FreezyLemon FreezyLemon marked this pull request as ready for review September 8, 2024 08:39
@FreezyLemon
Copy link
Contributor Author

Alright, I changed the things I wanted to and finally added the #[non_exhaustive] attribute to the error types. Should be good to go now

Copy link
Contributor

@YeungOnion YeungOnion left a comment

Choose a reason for hiding this comment

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

Gave it all a read, everything matches up and the language is consistent, thanks for this.

@YeungOnion
Copy link
Contributor

YeungOnion commented Sep 9, 2024

added a Send + Sync check like in #226

let me know if that works for you.

Co-authored-by: FreezyLemon <contact@freezylemon.com>
@FreezyLemon FreezyLemon force-pushed the experiment-separate-errors-for-new branch from eebdd03 to 6cd01db Compare September 9, 2024 11:14
@FreezyLemon
Copy link
Contributor Author

Yeah. I refactored it a bit.. I realized after 226 that it's trivial to do T: Sync + Send, only requiring one helper function which makes it a bit shorter.

@YeungOnion YeungOnion merged commit 59d0b71 into statrs-dev:master Sep 9, 2024
8 checks passed
@FreezyLemon FreezyLemon deleted the experiment-separate-errors-for-new branch September 9, 2024 15:03
@YeungOnion YeungOnion added this to the 0.18 milestone Sep 24, 2024
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.

2 participants