-
Notifications
You must be signed in to change notification settings - Fork 27
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
Release blank / nonblank model #228
Conversation
✅ Deploy Preview for silly-keller-664934 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #228 +/- ##
========================================
- Coverage 87.0% 87.0% -0.1%
========================================
Files 29 28 -1
Lines 1937 1931 -6
========================================
- Hits 1686 1680 -6
Misses 251 251
|
@pjbull this is ready for review. You'll want to read through this issue as well which tries to capture the implications of releasing a binary model on our other configuration options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update to remove "four" and just have plurals so we don't need to change the count of models in the future!
@@ -262,6 +302,8 @@ video_loader_config: | |||
confidence: 0.25 | |||
fill_mode: score_sorted | |||
n_frames: 32 | |||
image_height: 416 | |||
image_width: 416 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to new frame selection size (and maybe update default slowfast config?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docs change brings the default in line with what is actually used. Whether what is used is desirable is another question.
The issue is that the official configs get generated from what was used for training, which in this case, was 416 x 416. If we change the default slowfast config manually, this will get overwritten the next time we publish models (since the official configs are generated from the full train configuration used for training, with the machine specific params removed). If we want to use slowfast with a different frame selection size, we'll need to change that workflow.
I'm not super inclined to fix this as part of this PR which just aims to release the BNB model. It's still an open question if we continue to support the slowfast model and if we retrain it on more data, which could make this point moot.
2229139
to
ad1e16c
Compare
@pjbull I removed the number of models count and left a comment about the slowfast model (IMO nothing to do here, if we want different behavior, let's file an issue and do that separately). Ready for another look |
Adds a new official model:
blank_nonblank
. This was trained on both african forest and european data.Additional changes:
Outstanding:
Closes https://github.com/drivendataorg/pjmf-zamba/issues/130