Skip to content

compare indiv table rows and test #582

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

Merged
merged 1 commit into from
May 6, 2020

Conversation

petrelharp
Copy link
Contributor

@mufernando noticed that we can't compare IndividualTableRows because of the numpy arrays. And, it turned out this wasn't being tested anywhere. There is discussion about the right way to do this in attrs, but currently we just have to implement __eq__. The precise way to do this will change soon, but no big deal.

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #582 into master will decrease coverage by 1.18%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
- Coverage   88.49%   87.30%   -1.19%     
==========================================
  Files          20       21       +1     
  Lines       12170    16508    +4338     
  Branches     2267     3243     +976     
==========================================
+ Hits        10770    14413    +3643     
- Misses        788     1030     +242     
- Partials      612     1065     +453     
Flag Coverage Δ
#c_tests 88.49% <85.71%> (-0.01%) ⬇️
#python_c_tests 90.39% <85.71%> (?)
#python_tests 99.21% <85.71%> (-0.03%) ⬇️
Impacted Files Coverage Δ
python/tskit/tables.py 99.64% <85.71%> (-0.18%) ⬇️
python/_tskitmodule.c 83.97% <0.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 ea37280...650377d. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @petrelharp. Not quite up on the subtleties of what's the right approach here, though.

@petrelharp
Copy link
Contributor Author

Judging from that attrs github issue, I think this is fine.

@petrelharp petrelharp force-pushed the compare_table_rows branch from ae7a365 to e360507 Compare May 6, 2020 16:19
@petrelharp
Copy link
Contributor Author

Whoops - I missed some test coverage; now I should have got it?

@petrelharp petrelharp force-pushed the compare_table_rows branch from e360507 to 650377d Compare May 6, 2020 16:21
@petrelharp
Copy link
Contributor Author

I'll merge when the checks finish unless someone says otherwise.

@petrelharp petrelharp merged commit db53e69 into tskit-dev:master May 6, 2020
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