Skip to content

Make VonMises more flexible #33

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

Merged
merged 22 commits into from
Apr 17, 2025
Merged

Conversation

cscherrer
Copy link
Contributor

@cscherrer cscherrer commented Apr 11, 2025

Parameterized Trait Enhancement & VonMises Scaling Support

Summary

This PR introduces parameter structs for distributions and enhances the Parameterized trait with a default map_params implementation. It also adds scaling support for the VonMises distribution.

Changes

1. Parameter Structs

  • Added parameter structs for all distributions (e.g., BernoulliParameters, GaussianParameters)
  • Standardized parameter representation for better type safety and clarity
  • Updated all emit_params and from_params implementations to use these structs

2. Enhanced Parameterized Trait

  • Added a default map_params implementation that enables mapping functions over parameters
  • This provides a consistent way to transform distribution parameters

3. VonMises Distribution Improvements

  • Added Scalable trait implementation for VonMises distribution
  • Added map_params implementation for efficient parameter updates
  • Added from_parts_unchecked for more efficient construction

4. Data Module Enhancements

  • Added extensive tests for DataOrSuffStat and categorical datum
  • Fixed Scaled distribution representation by moving to eager computation of log Jacobian

Testing

Added comprehensive test coverage for all new functionality, including:

  • Edge cases for categorical data
  • Boolean conversions
  • Parameter mapping
  • VonMises scaling operations
  • Additional data module validation

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 62.53102% with 151 lines in your changes missing coverage. Please review.

Project coverage is 80.70%. Comparing base (6fd4b25) to head (eb4de19).
Report is 1 commits behind head on release/v0.19.0.

Files with missing lines Patch % Lines
src/dist/scaled.rs 18.33% 49 Missing ⚠️
src/data/mod.rs 83.00% 43 Missing ⚠️
src/dist/vonmises.rs 0.00% 33 Missing ⚠️
src/dist/crp.rs 0.00% 9 Missing ⚠️
src/dist/shifted.rs 0.00% 9 Missing ⚠️
src/traits.rs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           release/v0.19.0      #33      +/-   ##
===================================================
- Coverage            81.05%   80.70%   -0.36%     
===================================================
  Files                   98       98              
  Lines                19711    20081     +370     
  Branches             19711    20081     +370     
===================================================
+ Hits                 15977    16206     +229     
- Misses                3619     3760     +141     
  Partials               115      115              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cscherrer cscherrer changed the title crate::impl_scalable!(VonMises); Make VonMises more flexible Apr 14, 2025
@cscherrer cscherrer marked this pull request as ready for review April 17, 2025 17:31
Copy link
Contributor

@schmidmt schmidmt left a comment

Choose a reason for hiding this comment

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

This all seems reasonable. I like moving to named parameters too.

src/traits.rs Outdated
@@ -8,6 +8,19 @@ pub trait Parameterized {
fn emit_params(&self) -> Self::Parameters;

fn from_params(params: Self::Parameters) -> Self;

// TODO: If Self: Sized ok here? Should this be a requirement for the whole trait?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in every case the parameters should be sized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I changed it to

pub trait Parameterized: Sized {
    type Parameters;

    fn emit_params(&self) -> Self::Parameters;

    fn from_params(params: Self::Parameters) -> Self;

    fn map_params(
        &self,
        f: impl Fn(Self::Parameters) -> Self::Parameters,
    ) -> Self {
        let params = self.emit_params();
        let new_params = f(params);
        Self::from_params(new_params)
    }
}

@cscherrer cscherrer merged commit 6f58c99 into release/v0.19.0 Apr 17, 2025
2 checks passed
@cscherrer cscherrer deleted the cs-scalable-von-mises branch April 17, 2025 18:41
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