Add parsers for HRNet and AgeGender models #16
Conversation
|
For HRNet can we use or modify some existing parser? It would be good it's normalized if we are using normalized coordinates elsewhere. And then, for NCHW (OAK) vs NHWC (OAK4), we should be able to handle this. Might be worth opening a discussion on Slack whether we want this to be handled by DAI directly or somehow differently? I assume it's the same for all other parsers? |
I would leave it separate because it uses heatmaps. Moreover, other keypoint detection parsers are also separated. We could, however, think about joining them all together (but I'd do that in a separate PR)
Added in ec9ad5a.
True, we are currently solving this individually for each parser. Might be good to solve at the level of DAI (will open discussion on Slack) |
|
I'm opening this PR for review (cc @kkeroo, @klemen1999). Note that AgeGender model is a half-regression (age) half-classification (gender) model. I've currently put its message inside "misc.py" because we don't have the classification/regression messages in place yet. Once we merge the classification PR (#15) we can add this message to "classification.py". Moreover, we can keep the "misc.py" (or rename it to "mixed.py" maybe) and import there objects both from classification and regression messages (would make a separate PR for the second one). Thoughts? |
|
|
||
| age_gender_message = AgeGender() | ||
| age_gender_message.age = age | ||
| age_gender_message.gender_prob = gender_prob |
There was a problem hiding this comment.
Should we rather have gender (male or female based on threshold) rather than probabilities. It would give better info -> user does not need to know if 1 is male or female?
There was a problem hiding this comment.
I agree it would be more straight-forward but I also see added value of having the probability information. It might not be desired we abstract everything at the level of the parser not to make things too "simplistic" (it might help some users but be annoying for others who want to do something more advanced). Moreover, I think this message can be related to the classification message (once added) which will require probability values but also the class names (preventing the user from getting lost too much). So I'd leave it as it is for now (cc @klemen1999 on this also).
| """Initializes the AgeGenderParser node.""" | ||
| dai.node.ThreadedHostNode.__init__(self) | ||
| self.input = dai.Node.Input(self) | ||
| self.out = dai.Node.Output(self) |
There was a problem hiding this comment.
If going with the proposed change add parameter for thresholding?
Yeah agree, I wouldnt go into trying to abstract it too much right now but rather adjust the messages once new models are added. Potentially, in the future we might have Classification-regression msg having |
|
Related to classification messages: Would it make sense to first merge in #15 and then make changes to this PR so it uses |
kkeroo
force-pushed
the
feat/add_parsers
branch
from
5d7da5b
to
735d85a
Compare
kkeroo
changed the title
This PR proposed two additional parsers - HRNetParser and AgeGenderParser - and adds all the required support.