Skip to content

Conversation

@BigRedT
Copy link
Collaborator

@BigRedT BigRedT commented Aug 7, 2016

-examples/resnet_demo.py
-vis/pretrained_models/resnet

@kevjshih
Copy link
Collaborator

Since the demo is loading the pretrained model, shouldn't it be initializing the color mode to 'BGR'

@kevjshih
Copy link
Collaborator

Also I think a lot of the inference_fully_conv functions are identical to the ones in the inference.py, specifically bn and _max_pool. It would be better to simply import these from inference.py than to have duplicate implementations. You can also add a switch to the conv function to alternate between conv and atrous_conv to further reduce duplicate code.

@BigRedT
Copy link
Collaborator Author

BigRedT commented Aug 18, 2016

color_mode refers to the format in which data is fed in. The pretrained model is wrapped in a class which reverses the channels if RGB is fed in. Look at models.py

@kevjshih
Copy link
Collaborator

Ah, I misread that line in models.py.

@BigRedT
Copy link
Collaborator Author

BigRedT commented Aug 18, 2016

Yeah i can import bn, max_pool etc. Making a single conv with switch would require me to modify the code in many places and it's probably not worth the trouble.

@kevjshih
Copy link
Collaborator

kevjshih commented Aug 18, 2016

That's fair. Also the only difference between model_fully_conv and model is a few lines pertaining to avg_pooling. It's easier to just add a flag in model.py to simply disable the avg_pool and return an error if get_avg_pool is called. You would also need to add a switch to choose which version of inference to call.

@BigRedT
Copy link
Collaborator Author

BigRedT commented Aug 18, 2016

not really. there are more differences because i had to control the number of holes in atrous convs at each scale and also classification layer is made fully convolutional.

@kevjshih
Copy link
Collaborator

Right, but that doesn't affect model.py and model_fully_conv.py.

@kevjshih
Copy link
Collaborator

I don't know if this is bad practice, but you could basically do

if fully_conv:
import inference_fully_conv as inference
else:
import inference as inference

in the init function of the class if you had to

@BigRedT
Copy link
Collaborator Author

BigRedT commented Aug 18, 2016

Ah i see. That would remove lot of duplication. I will fix it.

@kevjshih
Copy link
Collaborator

Sounds good. Another option is to use some class inheritance if you think these two files will severely diverge in the future but still share a subset of functions.

@BigRedT
Copy link
Collaborator Author

BigRedT commented Aug 18, 2016

That's even better! Good call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants