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

replaces randsample with a custom-built randSample function and removes range #324

Merged
merged 10 commits into from
Jan 29, 2025

Conversation

alexhroom
Copy link
Collaborator

@alexhroom alexhroom commented Jan 21, 2025

This PR fixes part of #146 by removing the use of range from histp.m and replacing randsample from the Statistical Toolbox with a home-made implementation.

Note that of the six potential operations of randsample, this only implements four:

  • randsample(n, k) $\rightarrow$ randSample(n, k) (sample k random integers between 1 and n without replacement)
  • randsample(population, k) $\rightarrow$ randSample(population, k) (sample k random items from population without replacement
  • randsample(n, k, true, w) $\rightarrow$ randSample(n, k, w) (sample k random integers between 1 and n with replacement, with weights w)
  • randsample(population, k, true, w) $\rightarrow$ randSample(population, k, w) (sample k random items from population with replacement, with weights w)

It seems that we never do unweighted sampling with replacement, and randsample does not support weighted sampling without replacement, so I did away with the replacement parameter in randsample.

@alexhroom alexhroom force-pushed the 146-remove-randsample branch from 3d94b20 to 90dab94 Compare January 21, 2025 13:30
@alexhroom alexhroom force-pushed the 146-remove-randsample branch from 90dab94 to 65df06f Compare January 22, 2025 11:30
@alexhroom alexhroom marked this pull request as ready for review January 22, 2025 11:51
@alexhroom alexhroom changed the title replaces randsample with a custom-built randSample function replaces randsample with a custom-built randSample function and removes range Jan 22, 2025
@alexhroom alexhroom requested a review from DrPaulSharp January 27, 2025 11:26
Copy link
Collaborator

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

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

Looks good, there are just a couple of things I'd like you to have a look at please.

@DrPaulSharp DrPaulSharp self-requested a review January 27, 2025 15:00
@DrPaulSharp
Copy link
Collaborator

I don't think argument blocks work at all prior to 2022b unfortunately.

Copy link
Collaborator

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

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

All looks good, shame about the arguments block.

@alexhroom alexhroom merged commit 5196577 into RascalSoftware:master Jan 29, 2025
5 checks passed
@alexhroom alexhroom deleted the 146-remove-randsample branch January 29, 2025 17:34
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