Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Explicit class mapping in DetectNet #161

Merged
merged 1 commit into from
Jun 8, 2016

Conversation

gheinrich
Copy link

The hard-coded value in the code feels a bit too magical.

@lukeyeager
Copy link
Member

I agree. So shouldn't we remove that hardcoded value that you pointed out?

@gheinrich
Copy link
Author

I didn't dare to make that change :-)

Those who have been using DetectNet until now might want the default value to remain in place so they can keep using their old model description. Adventurers might want the layer to fail if no explicit mapping was specified. I think it's cleaner to not have the default value at all but I understand that this would break backward compatibility.

@lukeyeager
Copy link
Member

But merging this by itself would break things too, right? Since there would no longer be any data labelled as 1?

@gheinrich
Copy link
Author

gheinrich commented Jun 7, 2016

Sorry I don't understand the question. The intention behind this change is to explicitly do the same thing as is done implicitly i.e. assign class "1" to index "0" in the coverage tensor. Why would this break anything?

@lukeyeager
Copy link
Member

Oh I see, you're right. This is a noop change that exposes the underlying behavior more explicitly. I like that.

What I don't like is the default behavior when there are no class mappings. Currently, we only pick label 1 if there are no mappings. That seems illogical. We could either (1) only take label 0 or (2) take all labels and do multiclass by default. I'm not sure if (2) would actually work in practice.

@drnikolaev drnikolaev merged commit 846f3e4 into NVIDIA:caffe-0.15 Jun 8, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants