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

Hotfix v1.4.2 to resolve bug with ncells in feature dataframe #246 #247

Merged
merged 13 commits into from
Feb 18, 2023

Conversation

JuliaKukulies
Copy link
Member

Resolves #246

  • fixed bug in segmentation.py that assigned ncells of the last feature in each time step to all features in that time step
  • added a test to make sure that ncells is correctly assigned to different features
  • started to prepare this as a hotfix -> version bump and changelog
  • Have you followed our guidelines in CONTRIBUTING.md?
  • Have you self-reviewed your code and corrected any misspellings?
  • Have you written documentation that is easy to understand?
  • Have you written descriptive commit messages?
  • Have you added NumPy docstrings for newly added functions?
  • Have you formatted your code using black?
  • If you have introduced new functionality, have you added adequate unit tests?
  • Have all tests passed in your local clone?
  • If you have introduced new functionality, have you added an example notebook?
  • Have you kept your pull request small and limited so that it is easy to review?
  • Have the newest changes from this branch been merged?

@JuliaKukulies JuliaKukulies added the bug Code that is failing or producing the wrong result label Feb 16, 2023
@JuliaKukulies JuliaKukulies added this to the Version 1.4.2 milestone Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Base: 35.76% // Head: 39.22% // Increases project coverage by +3.46% 🎉

Coverage data is based on head (1507b40) compared to base (09b176b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           hotfix     #247      +/-   ##
==========================================
+ Coverage   35.76%   39.22%   +3.46%     
==========================================
  Files          15       11       -4     
  Lines        3403     2246    -1157     
==========================================
- Hits         1217      881     -336     
+ Misses       2186     1365     -821     
Flag Coverage Δ
unittests 39.22% <100.00%> (+3.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tobac/__init__.py 92.30% <100.00%> (ø)
tobac/segmentation.py 75.00% <100.00%> (-0.25%) ⬇️
tobac/tracking.py 65.45% <0.00%> (-3.34%) ⬇️
tobac/centerofgravity.py 6.25% <0.00%> (-1.22%) ⬇️
tobac/analysis.py 7.91% <0.00%> (-0.60%) ⬇️
tobac/wrapper.py 6.77% <0.00%> (-0.16%) ⬇️
tobac/utils/internal.py
tobac/utils/__init__.py
tobac/utils/general.py
tobac/utils/mask.py
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@w-k-jones w-k-jones 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. Comments are only minor changes that make no functional difference so can be left if you think that they are unnecessary

tobac/segmentation.py Outdated Show resolved Hide resolved
tobac/tests/test_segmentation.py Outdated Show resolved Hide resolved
Copy link
Member

@w-k-jones w-k-jones 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 to me, happy for this to be merged after second review

@w-k-jones
Copy link
Member

Also happy to merge with a single review if you don't have time to review this @freemansw1

Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @JuliaKukulies; sorry for my delay in reviewing, it's been a busy week.

@freemansw1
Copy link
Member

@JuliaKukulies @w-k-jones I'm happy for either of you to do the release. @JuliaKukulies it might be good for you to do one, if you're up for it!

@JuliaKukulies
Copy link
Member Author

@JuliaKukulies @w-k-jones I'm happy for either of you to do the release. @JuliaKukulies it might be good for you to do one, if you're up for it!

Thanks @freemansw1 and no worries! I am happy to do the release - only question is if the bug fixes in #250 or even #244 should be included or if we do the hotfix release just for this PR ?

@freemansw1
Copy link
Member

I'll leave that to @w-k-jones. I think at least #244 should be included, but I'll defer to his judgment.

@w-k-jones
Copy link
Member

Although I'd like to include #244 and #250 in 1.4.2, both changes were made on the v1.5 branch so I'm not sure how straightforward they would be to transfer

@w-k-jones
Copy link
Member

On second inspection, I think that both will be straightforward to transfer, so I'll make a new PR with the changes against the hotfix branch

@JuliaKukulies JuliaKukulies mentioned this pull request Feb 18, 2023
10 tasks
@JuliaKukulies
Copy link
Member Author

All right, merging this now. Note that I have already updated the changelog here, which you can have a final check on before merging hotfix into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code that is failing or producing the wrong result
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants