This repository has been archived by the owner on Aug 7, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add CLI params support + MobileNetV2 model + small refactoring #9
Add CLI params support + MobileNetV2 model + small refactoring #9
Changes from all commits
20e84cb
9e97981
ed52f60
0259253
ed97c35
22bed22
29c5a99
0754f06
3c07f54
645e594
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
I also do not get the name. Why
masked face net
?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.
Because it's the name of our current dataset. MaskedFaceNet :) Any suggestions are more than welcome
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.
If we are creating logic for creating data set for a specific solution (MobileNetV2) I would suggest to just create a separate solution to do this, rather than creating all for one class/function.
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 is indeed a good one. Do you have any idea how we could name that? We need to think about naming that kind of combinations of dataset and model. I can't come up with a name for a combination of MobileNetV2 and MaskedFaceNet :) Naming is indeed one of the hardest part of making software :P
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.
I think we shouldn't combine dataset with a particular model.
Rather than that we should keep a generic dataset prep here (connected only with the dataset we are using, parsing labels, etc).
Transforms specific for any model (or even generic ones for data augmentation) should be only applied here, not defined. It should be easy to do that if we pass all transforms as a function in arguments and apply it here.
Maybe even we should get a step further, merging both model-related transforms and data augmentation-related ones 🤔
Had not much free time last week, this one I'll try to come up with some nice file structure (after I'll finally decide on some collab tool).
If you want some inspiration you can check a template a friend of mine is doing (related to his thesis if I'm not wrong). Though I plan not to dive into it to be unbiased when preparing our template.
Also a quick observation - almost always when something is difficult to name it means we are doing something wrong 😅 If functions / modules are nicely separated then it shouldn't be as difficult (at least naming the modules / functions. Naming variables is and always be the major problem 😄 ).
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.
Hmm, probably it would be convenient to prepare some basic, semi-generic class for our DataModule.
It should define all the common operations for the task we have (image-processing), as probably most of them will be the same between different models.
Then we should extend this base class and overwrite specific methods for each Model we have (choosing correct datasets, defining some transformations etc).
What do you think?
This file was deleted.
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.
Having only loss and recall can be very misleading without having the precision as well.
Recall does not inform about any false positives. I suppose you have checked the dataset and if classes are properly balanced.
We have low loss value - though it does not mean anything. We do not know how to interpret this, or is loss
0.1
much better than0.2
. This is why usually additionallyaccuracy
is presented.On its own accuracy is very risky metric (without precision and recall data we could have a 99% accuracy when model is e.g. always outputting the same value, but classes were not balanced).
However, it describes what is the most important in evaluating the model outcome - how accurate the model is in the most intuitive way. If we see Recall / Precision / F1 score of e.g. 0.7 we need to think how to interpret it relating to the dataset, possible predictions etc. If we have an accuracy of 0.8 we already see that 80% of all predictions are correct. Then we can investigate other metrics to directly compare models, but that accuracy is the true goal anyways.
Also we miss precision. Here is a nice presentation what this could lead to (based on the situation from a shooting range):
I would opt for tracking all of these metrics (as they are not so slow to compute anyways). We never know when such information could become helpful.
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.
I think it would be nice to add some results for a tested classifier. Maybe in README? Without that kind of data, it's hard to say anything about a solution.
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.
I love that idea. Created separate README.md file in models directory. You could check this here:
mask_detector/models/README.md
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.
We could try to investigate if this generates some calculation efficiency cost. There could be the case that PyTorch is optimizing all the calculations in 1 Sequential layer, that we lose keeping separate Sequentials. Maybe it would be better to store them as arrays and then convert to 1 Sequential?
TBC (To Be Checked) though, it may be not a problem at all.