Refactor LayerParameter into per-layer Parameter messages #219
Conversation
|
@jeffdonahue great job! Although so far in a clean directory it doesn't compile, I guess due to the changes now other layers that assumed that some params were available in LayerParams now they are not. I will look into the code with more care, once I can compile it. |
|
Sorry about that, forgot to commit the last set of files I had to change after rebasing to get this to compile. It should compile with the commit I just pushed. Other than @sguada and anyone else making sure this works with their existing models, I still want to try a couple things out before we consider merging this - e.g. make sure finetuning an old model still works. |
|
Great, @jeffdonahue now code compiles and pass all test but the one about hdf5_data_layer @shelhamer I think in with this case and #209 we should merge them first in a different branch, and make sure everything works well before merging into |
|
Whoops, HDF5 test failure was a merge conflict problem I think. Passes with latest commit. |
|
@sguada I agree it is important to test how major PRs combine. You can check out a PR as a branch for testing with hub. You can then do merges and such with other branches to see how they integrate. Once we have tried the joint merges ourselves I think we can merge to |
|
Totally agree, @shelhamer I using hub as you showed me before to do that. What I meant is that we usually test PR in isolation, and that is fine, but for these 2 big ones, it will affect any other PR on the way. So we can either merge the other PR before and then adjust #219 and #209 or we will have to adjust all other pending PRs to the new formats. |
|
I vote for these core changes #219 and #209 going in first and other PRs @jeffdonahue, that should be less work for you so we can do our testing Le lundi 17 mars 2014, Sergio Guadarrama notifications@github.com a
|
|
Works for me (obviously) but am also happy to rebase and tweak on any other PRs you want to merge before. |
|
The back-merge of historical PRs to master into dev caused some conflicts (most likely in caffe.proto I'd imagine). |
|
Understand if we're still waiting to merge but FYI this and #209 (forward pass loss) are now rebased on dev. |
| } else { | ||
| LOG(FATAL) << "Unknown layer name: " << type; | ||
| case LayerParameter_LayerType_NONE: | ||
| LOG(FATAL) << "Layer " << name << " has unspecified type."; |
There was a problem hiding this comment.
should be fine, LOG(FATAL) causes a crash.
|
Looks great, I think we should go ahead and merge. |
|
#245's window data layer params will need to be added and the padding layer should be dropped, as discussed offline (Jeff's auto-conversion makes the deprecation unnecessary). |
|
@jeffdonahue please and name to |
|
@sguada huh? I will rebase this, add window data layer, do a bit of testing (beyond the existing unit tests), and merge - hopefully all today. |
|
Sorry Jeff, I meant to say that Blobs could have name, as a parameter. Which is defined by the layer. It will make things a bit easier to manage them. message BlobProto { |
|
Does the upgrade_net_proto also upgrade the prototxt files? Or only the network params in the proto file. |
| cropsize: 227 | ||
| } | ||
| name: "data" | ||
| type: IMAGE_DATA |
There was a problem hiding this comment.
Here there are missing the IMAGE_DATA params
|
@sguada great catch on the missing IMAGE_DATA params - I just added support for that layer type and I guess I missed its parameters. I'll fix that and step through every other layer type (again) to make sure I didn't miss anything else. Aside from that this actually needs a bit more work - I need to go through all the tools/*.cpp to make them all be backwards compatible with V0 net param files by calling the Net constructor which takes a filename string. (So far I only changed train_net's entry point - from grepping
Not sure what you mean by this - the changes to |
|
@jeffdonahue, I think we should provide the script to transform the prototxt and snapshots but I don't think we should change all tools/*.cpp be aware of the changes. A net should be able to be constructed given a Network params, although you can add the option to read it from a file. Yes I meant if changes to |
|
@sguada I'm confused why you don't want to upgrade the tools. @jeffdonahue's new I agree with Jeff's plan in #219 (comment) for finishing this. |
|
Hi Jeff, On Saturday, March 22, 2014, Evan Shelhamer notifications@github.com
Sergio |
IMAGE_DATA params
incorporate into util/upgrade_proto)
(which retains an optional V0LayerParameter field for legacy support) and LayerConnection renamed to LayerParameter
|
Almost perfect–but there's some chaff to clean up: 1322fa3 adds symlinks to caffe model params and solver state and `examples/imagenet/finetune*.sh scripts that I think are accidental. Please rewrite that commit so that these aren't versioned. |
|
I've read over everything and tests pass so I'll merge as soon as those files are dropped. Sweet work Jeff! |
|
Whoops, great catch - got lazy and |
Refactor LayerParameter into per-layer Parameter messages and add tools for seamless proto upgrade.
|
Lovely layers. |
Refactor LayerParameter into per-layer Parameter messages and add tools for seamless proto upgrade.
Addresses #208. This changes the way net protos are specified. Before:
After:
The most notable change is that we have a message, e.g.,
convolution_param. for each layer type that has its own parameters. I also turned the layer types from a string into an enum to catch spelling errors etc. at compile time.This should be fully backward compatible with old model protos - the
Net::Net(const string& param_file)constructor will attempt to load the param_file using the new proto specification, but if that fails will instead try to load it with the old ("v0") proto specification and then convert it to the new format at runtime, printing a warning that you should convert your nets to the new format.There is also a script
tools/upgrade_net_proto.cppwhich converts the a v0-formatted proto to the new format and saves the new format to a file.Both the converter script and the Net constructor will first pass the v0 formatted proto through a function
UpgradeV0Paddingthat turns padding layers into pad-aware conv layers (assuming they are passed into conv layers; errors if not), and then another function that upgrades to the new proto format.