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

Weights for Univariates #46

Merged
merged 5 commits into from Sep 21, 2023
Merged

Weights for Univariates #46

merged 5 commits into from Sep 21, 2023

Conversation

itsdebartha
Copy link
Contributor

This PR partially addresses #23 and adds weights for univariate average-shifted histograms.
Jobs done:

  • Create a new function ashw for univariate weighted average-shifted histograms
  • Add tests for them
  • Update the documentation

Adding the functionality of weights in average shifted histograms for univariates
Added documentation for weights
@ParadaCarleton
Copy link
Contributor

@joshday

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 72.72% and project coverage change: -1.14% ⚠️

Comparison is base (b6d2989) 84.68% compared to head (5fb7ecc) 83.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   84.68%   83.54%   -1.14%     
==========================================
  Files           4        4              
  Lines         209      231      +22     
==========================================
+ Hits          177      193      +16     
- Misses         32       38       +6     
Files Changed Coverage Δ
src/AverageShiftedHistograms.jl 100.00% <ø> (ø)
src/univariate.jl 75.80% <72.72%> (-0.67%) ⬇️

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

@joshday
Copy link
Owner

joshday commented Jul 26, 2023

Looks cool! Give me a chance to play around with the PR, but I'd love to add this feature 👍 .

@ParadaCarleton You don't need to ping me (especially within the same day as the PR!).

@joshday
Copy link
Owner

joshday commented Aug 2, 2023

This is a great start, but there's a few changes I'd like to see:

  1. Make weight a keyword argument (can default to nothing) to the ash/ash! functions.
  2. Allow non-integer weights. This would require parameterizing the Ash struct with the eltype of the counts field.

I think 1 is what I'd require before merging. 2 Can happen sometime later (by me, you, or anyone else).

Changed the weights to keyword argument. Updated documentations and tests accordingly
@itsdebartha
Copy link
Contributor Author

I made the requested change (only 1). Is it ok now?

@joshday
Copy link
Owner

joshday commented Aug 2, 2023

I meant to remove ashw in favor of:

ash(x; weight=nothing, kw...)

- Removed functions `ashw` and `ashw!`
- Integrated the weighted-ash as part of `ash` and `ash!`
- Updated tests
@itsdebartha
Copy link
Contributor Author

I have made the changes you requested (again, only for 1). Let me know what you think about this now...

P.S.: Sorry for having to wait so long...

- Implement AbstractWeights
- Added tests for weights, aweights, fweights, pweights
- Removed weight as a keyword argument and make it a positional argument instead
- Updated documentations
@itsdebartha
Copy link
Contributor Author

I made some modifications to the previous commit to include AbstractWeights with a lot of help from @ParadaCarleton ...

@joshday
Copy link
Owner

joshday commented Sep 21, 2023

Looks good to me. Thanks for the PR and responding to comments!

@joshday joshday merged commit 96d5b33 into joshday:master Sep 21, 2023
10 of 13 checks passed
@itsdebartha itsdebartha deleted the weights branch September 22, 2023 04:15
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.

None yet

3 participants