Skip to content

Conversation

@hyanwong
Copy link
Member

No description provided.

@hyanwong
Copy link
Member Author

Tests are very minimal, but I can't think of any corner cases, really.

@jeromekelleher
Copy link
Member

It'd be nice to do a bit more testing. To pass these tests, all I'd need to do is have

@property
def span(self):
     return 1.0

@hyanwong
Copy link
Member Author

hyanwong commented Sep 3, 2020

Like this? I simply changed it to test a few trees.

self.assertGreater(ts.num_trees, 1)
breakpoints = list(ts.breakpoints())
self.assertEqual(breakpoints[0], 0)
self.assertAlmostEqual(breakpoints[-1], ts.sequence_length)
Copy link
Member

Choose a reason for hiding this comment

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

Should be exactly equal, shouldn't they? We'd only expect numerical differences if we've performed some calculations on the values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine. I wasn't sure how the breakpoints were calculated, but if the last one is simply passed through from the seq length, then that should be fine. I'll change it.

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.

Looks good, thanks @hyanwong. Minor comment.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

Sorry @hyanwong I missed the notifications on your push. Looks good modulo JK's comment.

@hyanwong
Copy link
Member Author

Should be done now

@mergify mergify bot merged commit e3b9598 into tskit-dev:master Sep 11, 2020
@hyanwong hyanwong deleted the span-for-interval branch September 11, 2020 13:08
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.

3 participants