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

Added merge/split tests #1

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

freemansw1
Copy link

The main purpose of this PR is to add merge/split tests, but I've also done a couple other things.

First, I've imported merge_split in the main __init__.py, which both builds the documentation and allows users to import tobac and then run tobac.merge_split instead of having to import tobac.merge_split explicitly.

Second, I've changed the default start track number to 0 from -1. I think this caused a great deal of confusion over on the main PR, and threw me for a loop while building the test.

Third, I've added a test that merges two cells and checks to make sure that they are merged (with an appropriate distance set) or not merged (with a distance set too close). I think that this gets us most of the way there on tests, if not all of the way there. I think a figure would help us out with the documentation, and I'm working on that now.

@kelcyno, I'd love to hear your feedback. I'm happy to send along the notebook that I used to create the test. The idea and some of the code for this test comes from @JuliaKukulies and @snilsn.

@freemansw1
Copy link
Author

@kelcyno can you approve the workflows when you get a chance so that I can see what this does for coverage?

@kelcyno
Copy link
Owner

kelcyno commented Oct 31, 2022

@freemansw1 I need to double check, but by having the count begin at -1, it ensures that all features that are not linked to a cell also are not linked to a track. Features that are left un-linked have a cell id of -1, and I think that should be the same case with Tracks. BUT I think the correct order here is to begin counting at 0, and if the id == -1, then the track id -1 is given.

kelcyno pushed a commit that referenced this pull request Oct 31, 2022
kelcyno pushed a commit that referenced this pull request Oct 31, 2022
merge in latest tobac changes
@freemansw1
Copy link
Author

@freemansw1 I need to double check, but by having the count begin at -1, it ensures that all features that are not linked to a cell also are not linked to a track. Features that are left un-linked have a cell id of -1, and I think that should be the same case with Tracks. BUT I think the correct order here is to begin counting at 0, and if the id == -1, then the track id -1 is given.

Ok, interesting. Before making this change, I was definitely getting a single track with an id of -1, which I assume is not the intended behavior.

@freemansw1
Copy link
Author

I'm happy to (or you can) change it back to starting at -1, but I was getting incorrect behavior before.

@freemansw1
Copy link
Author

Okay, absent codecov.io working, I ran a coverage test on my machine. This test covers most of the lines in merge_split, and I'm reasonably happy with this for the main tobac repo. We'll need to do some user communication to encourage users to add networkx, and ultimately we may decide to add it to our requirements, but I think this we are in a good place for 1.4.0.

@kelcyno if you are happy with my changes, I think we can turn this around to a quick approval and get 1.4.0 released early next week.

@kelcyno kelcyno merged commit 8412ba4 into kelcyno:merge_split Nov 5, 2022
kelcyno pushed a commit that referenced this pull request Feb 9, 2024
Add removal of parent features at any lower threshold
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