Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor data layers to avoid duplication of data transformation code #954

Merged
merged 2 commits into from
Aug 22, 2014

Conversation

arntanguy
Copy link
Contributor

As discussed in #941, I started refactoring the data layers to avoid the current code duplication of data
transformations.

  • I ceated a new protobuf message TransformerParameter that contains the transformation parameters
    (crop, mirror, mean file...)
  • I created a new class DataTransformer that takes care of applying the transformation defined in the protobuf file to the data
  • I refactored DataLayer to use this new class.
  • I updated the code to update the old network definition files to the new one
  • I updated the unit tests

Could @shelhamer or someone else could have a look at it before I go about refactoring the other
data layers?

@sguada
Copy link
Contributor

sguada commented Aug 20, 2014

@GeenuX Thanks for your initial commit, could you also update the ImageDataLayer to use the same DataTransformer?

@bhack
Copy link
Contributor

bhack commented Aug 20, 2014

@sguada Is this superseding #569?

@jeffdonahue
Copy link
Contributor

Thanks for working on this @GeenuX, it will be great to have the madness that is the current set of DataLayers cleaned up.

Since commenting on @sguada's #569 I've changed my mind a bit...I think each of the transformations actually could be done as layers with little or no extra cost in terms of memory/time. The key would be to use @longjon's Blob::set_data to make the input blob point directly to the data field of the BlobProto, thereby avoiding any additional explicit copying. set_data would need to be called for each input datum, but it's cheap as it just modifies a pointer.

The current design looks good and would definitely be an improvement over the current state as well, but is slightly less flexible than having each transformation be a layer. We could modify this to use layers later but might end up having to make proto-breaking changes, which isn't ideal. I'll leave it up to @GeenuX to decide if/how to take this suggestion.

Start the refactoring of the datalayers to avoid data transformation
code duplication. So far, only DataLayer has been done.
@arntanguy
Copy link
Contributor Author

I've refactored the ImageDataLayer as well.

I agree, it would be nice to have the transformations defined as separate layers. I'm not sure
I'll have the time to look into it though. In the meanwhile, I think this ought to be merged, as it gets rid of all the duplicated code between Data and Image layers.

@shelhamer shelhamer self-assigned this Aug 21, 2014
shelhamer added a commit that referenced this pull request Aug 22, 2014
Refactor data layers to avoid duplication of data transformation code
@shelhamer shelhamer merged commit 4267e47 into BVLC:dev Aug 22, 2014
@shelhamer
Copy link
Member

@GeenuX I agree this is a worthwhile improvement in itself. Thanks for your further work to improve the data layers and make data processing more sane.

@shelhamer
Copy link
Member

@sguada I will let you decide how to carry on in #569. While the DataTransformer is a step in the right direction, it would be better still to have transformation layers along the lines of #954 (comment).

I will follow-up by refactoring WindowDataLayer and MemoryDataLayer; in the latter's case I will add data transformations and expose its use for deployment nets.

@shelhamer
Copy link
Member

Don't panic, but this breaks model definitions in dev as pointed out by @jeffdonahue.

I will follow up shortly with a fix that #963 fixes this by

  1. restoring the prototxt field numbers: these should never change!
  2. adding a data layer parameter -> transform parameter upgrade path to Net::Init()
  3. updating the model definitions to the new format

Sorry for merging before correcting these issues.

@arntanguy
Copy link
Contributor Author

Sorry about that! Thanks for the fix!

This was referenced Sep 18, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Refactor data layers to avoid duplication of data transformation code
RazvanRanca pushed a commit to RazvanRanca/caffe that referenced this pull request Nov 4, 2014
Refactor data layers to avoid duplication of data transformation code
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.

5 participants