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

Use "area under the curve" in `shade_p_value()` #229

Merged
merged 8 commits into from Apr 8, 2019
Merged

Conversation

@echasnovski
Copy link
Collaborator

echasnovski commented Apr 7, 2019

This PR closes #213.

Tests for shade_p_value() moved to separate file. They now use data that is precomputed in 'helper-data.R' file.

@echasnovski echasnovski requested review from andrewpbray and ismayc Apr 7, 2019
@echasnovski echasnovski mentioned this pull request Apr 7, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 7, 2019

Codecov Report

Merging #229 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #229   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           13     14    +1     
  Lines         1101   1155   +54     
======================================
+ Hits          1101   1155   +54
Impacted Files Coverage Δ
R/visualize.R 100% <100%> (ø) ⬆️
R/shade_p_value.R 100% <100%> (ø)
@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Apr 7, 2019

I've also added fixing of random generation method in order to solve the problem of test reproducibility on r-devel. In 3.6.0 there is a change in a default way of how sample() works (see this link). So to fix tests I forced the "3.5.0" behavior.

By the way, @ismayc , this change in 3.6.0 might also be the reason why you had to fix tests in a3262a2.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Apr 7, 2019

Thanks for the info. Might be best to just shift to using fixed rows instead of generating at random down the road then for the tests?

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Apr 7, 2019

Yeah, sure. That is probably a more tidy way.
I just wanted to pass the information for which I recently had to debug quite some time :)

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Apr 7, 2019

Appreciated :)

@andrewpbray andrewpbray merged commit 15ea3c7 into develop Apr 8, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@andrewpbray

This comment has been minimized.

Copy link
Collaborator

andrewpbray commented Apr 8, 2019

Thanks @echasnovski !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.