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

Data sampler: Allow selecting 100% of data, shuffle it #3727

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
3 participants
@janezd
Copy link
Contributor

commented Apr 5, 2019

Issue

Closes #2035.

Description of changes

#2035 required:

  1. Modified DataSampler that allows selecting all instances (not just n-1)
  2. DataSampler doc mentioning that the widget shuffles the dataset
  3. A test that checks that DataSampler really shuffles the dataset
  4. An example in Test & Score docs showcasing how this can be done (like #3245)

I've done them all, but I didn't have a good use case in 4. @ajdapretnar, in an already agitated state because of certain issues around #3722, told me in no uncertain terms to remove it. I'm married for 17 years, so I know better than to complain. On the contrary, I must say that I didn't like that text either, so I was very happy to remove it.

Documentation still explains that choosing the entire data set shuffles it, which, I must say, should suffice.

(Joke aside, it indeed does.)

Includes
  • Code changes
  • Tests
  • Documentation
@astaric

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Removed requirement 4 from the comment, as I do not remember anymore what the intended use case was.

@codecov

This comment has been minimized.

Copy link

commented Apr 5, 2019

Codecov Report

Merging #3727 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3727      +/-   ##
==========================================
+ Coverage   84.53%   84.54%   +<.01%     
==========================================
  Files         373      373              
  Lines       68472    68490      +18     
==========================================
+ Hits        57886    57902      +16     
- Misses      10586    10588       +2

@thocevar thocevar merged commit 80e4080 into biolab:master Apr 12, 2019

5 checks passed

codecov/patch 100% of diff hit (target 95%)
Details
codecov/project 84.54% (+<.01%) compared to 08da18a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.