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

Add sort and remove duplication to statistical_inefficiency #119

Merged
merged 7 commits into from
Apr 14, 2021

Conversation

xiki-tempula
Copy link
Collaborator

Fix #118

@codecov
Copy link

codecov bot commented Mar 27, 2021

Codecov Report

Merging #119 (65c4066) into master (0b31fb2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   97.34%   97.39%   +0.05%     
==========================================
  Files          16       16              
  Lines         942      961      +19     
  Branches      206      211       +5     
==========================================
+ Hits          917      936      +19     
  Misses          5        5              
  Partials       20       20              
Impacted Files Coverage Δ
src/alchemlyb/preprocessing/subsampling.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b31fb2...65c4066. Read the comment docs.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Overall this looks sensible to me. Maybe @dotsdl wants to have a quick look.

I mainly have comments on the tests.

Also add an entry to CHANGELOG, please.

src/alchemlyb/tests/test_preprocessing.py Outdated Show resolved Hide resolved
src/alchemlyb/tests/test_preprocessing.py Outdated Show resolved Hide resolved
@xiki-tempula
Copy link
Collaborator Author

@orbeckst Thank you for the review. I have updated the PR.

@orbeckst
Copy link
Member

orbeckst commented Apr 9, 2021

@xiki-tempula , this looks good to me. But do you know why the coverage is so low? I see that two if statements are not fully checked (essentially, only the if True code path is tested while the effective else: pass is not). Under which circumstances would these other code paths be triggered?

@xiki-tempula
Copy link
Collaborator Author

@orbeckst Thank you for the review. The two if checks if the series is given and will only modify the series if the series is given.
If the series is not given, the data will be sorted and duplications removed but not decorrelated.

@orbeckst
Copy link
Member

Then these should be simple tests: just as before, just don't give the series. It seems a bit over the top but could you please add tests for this case, too? Then this PR will be done.

@xiki-tempula
Copy link
Collaborator Author

@orbeckst Thanks for the suggestions, I have bumped the test.

@orbeckst orbeckst merged commit 950b591 into alchemistry:master Apr 14, 2021
@xiki-tempula xiki-tempula deleted the sort branch April 15, 2021 07: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.

Adding automatic sorting and duplicate removal
2 participants