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

WIP: Add Background Subtractors. #106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yuvallanger
Copy link
Contributor

Following up on issue #105.
Does it look sane to you?
Please do not pull yet. I want to add more of its methods.

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 20, 2019

So far so good.

P.S. Hint of the day: if you aren't ready yet start PR name with WIP: 🙂

@yuvallanger yuvallanger changed the title Add BackgroundSubtractorKNN and its creator. WIP: Add BackgroundSubtractorKNN and its creator. Feb 20, 2019
@yuvallanger
Copy link
Contributor Author

Sorry for imposing. Got advice on writing tests? @Pzixel

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 20, 2019

just write them. I'm not sure, there is not special advices here. Setup-act-assert, classic loop

@yuvallanger yuvallanger force-pushed the background-stubtractor-knn branch 3 times, most recently from 17b287c to 33a8773 Compare February 22, 2019 15:48
@yuvallanger yuvallanger changed the title WIP: Add BackgroundSubtractorKNN and its creator. WIP: Add Background Subtractors. Feb 22, 2019
@yuvallanger yuvallanger force-pushed the background-stubtractor-knn branch 2 times, most recently from 3332f9c to c49b466 Compare February 23, 2019 13:35
@yuvallanger
Copy link
Contributor Author

Biggest patch I had ever wrote. What do you think, @Pzixel? I wrote some tests, too!

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 23, 2019

Looks great.

I can't look at it right now, but I gonna come back later.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)


Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

@yuvallanger
Copy link
Contributor Author

Looks great.

I can't look at it right now, but I gonna come back later.

Thank you.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)

I didn't realize that. I will look and see what's what tomorrow.

Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

The following is one part thinking out loud and one part asking for advice:

I will create another branch to work on, then merge with this one.
I don't know if I should keep, squash, or fixup all the tiny bug fixing commits along the way. Maybe I should just let it accumulate and decide right at the end. I will commit fixup ones with message "f".

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 23, 2019

Just keep pushing commits as they go. There is no shame having ~30-40 commits with no sane message since they anyway will go away. Example from my private repo

image

becomes

image

When merging

@yuvallanger
Copy link
Contributor Author

I do not understand the appveyor test failure.
Locally it is running fine. My OpenCV version is 3.4.5.

Got any ideas on how to find the reason it had failed?

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 24, 2019

Look at output:

    Running `C:\projects\cv-rs\target\debug\deps\test_video_analysis-3a0f73e01162fd28.exe`
running 6 tests
test knn_tests::test_create_background_subtractor_knn_apply ... ok
test knn_tests::test_create_background_subtractor_knn_default ... ok
test mog2_tests::test_create_background_subtractor_mog2_apply ... ok
error: test failed, to rerun pass '--test test_video_analysis'
Command exited with code -1073741819

Considering it's only failing on Windows it's some windows-specific problem. Either it's something windows can't run (then it should be hidden via cfg) or there is some bug linux is not affected of.

@yuvallanger
Copy link
Contributor Author

Should I add something like #[cfg(not(windows))]? Wouldn't that mask the missing coverage?

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 25, 2019

Firstly you should figure out what's wrong on Windows. It's like 99% chance that you're doing something wrong: e.g. if you do Command::new("curl") to make an HTTP request it will work in linux, but won't on windows.

@yuvallanger
Copy link
Contributor Author

The furthest I can go in making it work on Windows is generalizing the path. I don't have a Windows machine I can use.

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 27, 2019

I wouldn't be able to run it on windows till next week.

You probably could check it on windows VM.

In worst case we have to configure it out via cfg and open "up to grabs" issue, but I hope it won't be required.

@vadixidav
Copy link

I can follow through on this PR after #117 is merged.

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