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

implement as a CLAMS app #14

Closed
keighrim opened this issue Oct 17, 2023 · 8 comments
Closed

implement as a CLAMS app #14

keighrim opened this issue Oct 17, 2023 · 8 comments

Comments

@keighrim
Copy link
Member

This issue to track progress on wrapping the classification model into a CLAMS app (app.py + metadata.py).

@marcverhagen
Copy link
Contributor

See the pull request at #25 for progress

@keighrim
Copy link
Member Author

Initial implementation put a lot of burden on classify.py especially regarding smoothing process. However, many configuration for smoothing (sampling rate, time-wise threshold, probability-wise threshold, etc) should be re-implemented as runtime parameters. Namely we need to either

  1. Leave self.classifier object initiation in the "app" __init__ and move out smoothing from classify.py to _annotate() (in app.py)
  2. Move classifier object creation to _annotate() method
    so that the smoothing can be configured by runtime params passed to _annotate().

(I believe option 1 would be much cheaper computationally)

I've been spending time on re-writing much of feature extraction and classifier configuration code (#28 #31 #27), but I'm stuck now as I'm having a hard time understanding the current implementation of the smoothing in the classify.py.

@marcverhagen
Copy link
Contributor

Hmmm, I don't think I follow the reasoning. I do agree that many settings need to be parametrized, but we wanted to roll out a proof-of-concept prototype soon, and as far as I was concerned having some baked-in default settings for the smoothing would have been fine.

Changing classify.py so that it takes arguments (replacing the module-level variables) would not be hard to do.

@marcverhagen
Copy link
Contributor

One more comment, to explain the first sentence of my previous comment. We can parametrize any setting as long as it makes sense to do that (that is, it is beneficial to the end user, or to us, but mostly to the end user).

What I do not see is why that means that smoothing has to move somewhat along the lines of the 2 suggestions. I also don't see why one of the two suggestions is cheaper than the other. If the code in classifier.py is set up properly then it can simply be called with any settings taken from parameters of the app.

Also, if smoothing lives in the app alone than the standalone non-app version will not have any smoothing.

@keighrim
Copy link
Member Author

For example, I'm planning to use the app just for credits and chyrons, but with loosened "score" thresholds, to generate image examples for RFB annotations. But I can't do that with current implementation without editing the code.

Also I don't think that the end users (and their use cases) are the only stakeholders. We also need to consider structuring the codebase for future developers. The "model" we trained from the annotated data is only doing image classification, no time involved (it uses positional features, but it's all pre-computed, so that doesn't make it a sequential model). I think conglomerating the image classifier component and the time-based smoothing component into one "classifier" class will result in unnecessary coupling between components that can be completely independent, and add avoidable complexity to future iteration of the app development.
(smoothing doesn't have to be in app.py, and can be a new module under modeling package and providing python apis to app.py)

@marcverhagen
Copy link
Contributor

Thanks for the details, that makes much more sense to me than the earlier post. The code for smoothing is separate from the code that does the classification, although it does live in the same module. There is no strong coupling beyond being in the same module and creating two modules should not be a big deal. In fact, most of the classifier module is about generating time frames from classifications of frames, probably another name is in order.

I was not saying that end users are the only steak holders. Structure the code base for future developers is obviously desirable, but that is a way bigger issue than just deciding what the parameters are.

And I am still wondering why this should all be in the first published version.

@keighrim
Copy link
Member Author

keighrim commented Dec 8, 2023

Hi @marcverhagen , this issue is now being a major bottle neck for additional RFB annotation, since I plan to use the new SWT to generate RFB annotation source files. Can we expect the app is fully ready to load the "best" model with complex binning and positional encoding anytime soon? I'd really like to continue RFB work as soon as possible, although at this point, it seems there's no chance we can meet the December deadlines (including LAW) for submitting the planned RFB paper with enough data annotated.

@marcverhagen
Copy link
Contributor

Okay, I will make it my priority.

@github-project-automation github-project-automation bot moved this from Todo to Done in apps Dec 20, 2023
@clams-bot clams-bot reopened this Feb 7, 2024
@github-project-automation github-project-automation bot moved this from Done to Todo in apps Feb 7, 2024
@keighrim keighrim closed this as completed Feb 9, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in apps Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants