Added opencv vector<Mat> to memory data layer with tests

[mtamburrano]

• AddMatVector to load Opencv Mat into MemoryDataLayer
• MemoryDataLayer accepts blobs with dynamic batch size, especially useful when you need to predict a number of images that is not known a priori

[beniz]

Thank you, very useful and works fine.

[mtamburrano]

@pleasurelong dataset.hpp is in caffe/include/caffe/dataset.hpp. I don't know why you don't have it, are you sure you have not accidentally deleted it?

[ghost]

@mtamburrano This is OK, I do not need this file now. thanks for your replay ^ ^

[bhack]

@sguada probably this will need to be changed if "the mergers" pass your #1328. What do you think? Could we unify this two?

[beniz]

@bhack my 2 cents here, this short PR is very simple and works well against the current dev tree. It seems to be of interest to several users. To me it'd make sense to not delay it, and update it in other PR, as needed.

[bhack]

@sguada Can you make a passage through here?

[sguada]

Maybe there was a mistaken approach with AddDatumVector which now is translated to AddMatVector, so let's fix both.

The idea is that MemoryData layer can hold more data than needed by batch_size and that forward passes consumes part of the queue data.

So let's rename added_data_ and added_label_ to data_queue_ and labels_queue_ to make more clear the usage.

If you want to add dynamic batch_size we could add a method for that, which would require to resize the tops.

[mtamburrano]

so, what about when added_data_ (or data_queue_) has size < batch_size? The forward pass should not do anything until the queue is large enough or it should be considered as an error?

[sguada]

I think that should be an error, data_queue_ should be a multiple of batch_size. So if one wants to add less data should change first the size of batch_size.

[mtamburrano]

Ok then, just to recap:

• rename added_data_ and added_label_ to data_queue_ and labels_queue_
• the forward pass should take as much items as batch_size, until the data_queue_ is empty
• introuduce a new method to automatically resize batch_size to the dimension of data_queue_ and reshape tops accordingly. In this case we could allow initial batch_size to be larger than queue_data_ size?

Doubt:

If we allow queue_data_ to be a multiple of bath_size, the user should call forward until the data is empty. So we should create also a method to check if queue_data_ is empty?

[sguada]

The forward pass already takes batch_size elements and move the current pos to the appropriate position. Currently when it gets to the end loop back to the beginning.

Replies:

• Yeah rename vars
• No, the forward doesn't need to know about data_queue it just has move the current pos within data_ which points to data_queue if that was the case.
• No, the batch_size should not change depending of the size of data_queue_, it is defined as param of the layer. What I said is that we could allow to change it after construction by exposing another method, i.e. ChangeBatchSize(int batch_size) which will cause a resize of the top, which always has to be equal to batch_size.

Since the user pass the data should know how many forward passes have to do, but yeah you can also add a bool var processed_data that is true when pos reach the end n_, and exposed with a public method.

[mtamburrano]

yes, I meant what you write, I probably badly explained it.
My doubt is, has ever worked this mechanism? I mean, when you add more datum than batch_size and consume it with multiple forward passes?
I ask this because I see that the queue is implemented, but the check at https://github.com/BVLC/caffe/blob/dev/src/caffe/layers/memory_data_layer.cpp#L37 doesn't allow to add datum larger than batch size...
So what, I think that the check should just verify if datum.size() is a multiple of batch_size like https://github.com/BVLC/caffe/blob/dev/src/caffe/layers/memory_data_layer.cpp#L57.

Additionally the line 49 Reset(top_data, top_label, batch_size_); should not be Reset(top_data, top_label, num); ?
Although I don't understand the usefulness of n_, it is only used to check in Reset if batch_size is a multiple of n_, but n_ == batch_size because the issue reported above, and even fixing the issue like I proposed, the check in Reset would be redundant because would be performed in addDatumVector

[sguada]

@mtamburrano MemoryDataLayer was originally developed by @longjon to be able to pass data directly in memory by calling Reset with pointers to the data and labels, with sizes that could be multiple of batch_size. However later, I think @kloudkl, added the option to also pass Datum, and vectors of Datum, adding the extra constraint that the size of the vector should be the size of the batch.

This layer when the data is added through Reset, it works nicely with multiples of batch_size. So let's remove that constraint from AddVector for both Datum and CvMat, and allow the layer to add more than 1 batch at the time.

[sguada]

This code should be surrounded by #ifndef OSX

[mtamburrano]

restructured MemoryDataLayer following @sguada's directions:

• added_data and added_labels now can be store multiple of batch_size elements, and are correctly reshaped.
• the forward pass consumes batch_size elements.
• data can't be added before the older data has not been completely consumed.
• when all the data has been consumed, the batch_size can be changed with ChangeBatchSize method.

[bhack]

Can be merged now?

[StevenLobo2]

What could be alternative for OS X?

[sguada]

Define Reshape method to do the reshape of the tops, and use it in DataLayerSetUp. Remove the reshaping from the Forward since Reshape method is called by the net before calling Forward.

[mtamburrano]

I think this is not needed anymore because #1313, right?

[longjon]

I haven't read this carefully, but since I've been asked about Reshape:

• In my conception the Reshape method is for computing top shapes as a function of bottom shapes. So, a data layer has no bottoms, and needs no Reshape method. This is purely an organizational distinction.
• Nevertheless, all layers are allowed to reshape their top blobs in their forward passes. So a data layer that produces top blobs of various shapes can reshape in Forward.
• Reshaping is basically free, there's basically no reason ever to have a bool like needs_reshape_.
• I can't tell whether ChangeBatchSize is really needed here, but if it is it should probably be called set_batch_size, since it's basically a setter.
• I don't see any way that this should interact with Reshape single input batches for inputs of varying dimension #1313 since this derives from BaseDataLayer and that only affects DataLayer.

[bhack]

@longjon Thank you for the feedbacks. It was not claimed an interaction with #1313 at code level but only for maintaining "semantic constancy" inside the caffe code caused by divergent comments between core members in two different PRs. We are waiting a final reply from @sguada so that we can allocate working time to the last actions needed to let this merge.

[sguada]

ChangeBatchSize should be just a setter, don't need to reshape anything else. That will happen in the corresponding methods.

[sguada]

To align with #1313, just follow the directions given by @longjon:

• Reshape add_data and add_label to match the added vector
• Reshape the top in the Forward to match the batch_size
• Change the name to set_batch_size

[bhack]

@Nerei do you have any hints on how we could remove ifdefs for OSX introduced by the inclusion of opencv in caffe? This will also reply to @StevenLobo2 question.

[mtamburrano]

Edits done, I hope this time everything is fine ;)

[bhack]

@deepcnn I don't see where do you are feeding the memory data layer. Please ask usage questions on the caffe-users mailing list. Thanks!

[bhack]

@shelhamer @mlapin Can we remove OSX opencv ifdef from here now that #1236 is merged?

[shelhamer]

I think so, as long as you follow #1236 in removing OpenCV includes from
headers.
On Thu, Jan 15, 2015 at 23:28 bhack notifications@github.com wrote:

@shelhamer https://github.com/shelhamer @mlapin
https://github.com/mlapin Can we remove OSX opencv ifdef from here now
that #1236 #1236 is merged?

—
Reply to this email directly or view it on GitHub
#1416 (comment).

[bhack]

@deepcnn Please use your thread in the mailing list. We are not talking about your issue here. Actually this PR cannot work on OSX caused by OSX ifdef wrapping.

[bhack]

@shelhamer I think this is ready and reviewed multiple times by @sguada and partially by @longjon. We don't have OSX building infrastructure in our team and on Travis. Can we merge this? Some core developers with OSX want to test the removal of ifdefs?

[bhack]

@shelhamer I think that you have OSX. Can you brew this iced caffe or can we merge this PR?

[shelhamer]

@bhack I'll take a look after the ICML deadline 02/06 -- in other news, CUDA 7 is at last compatible with libc++ so the OS X installation for 10.9 + 10.10 will be so much simpler. I'll check how this works with pycaffe at the same time so we can see about having an interactive solving example.

[bhack]

@shelhamer We can eventually allocate some working time for the week after 02/06 (we are waiting for this from CVPR deadline and we have also others PR in the review queue almost stalled). I really understand research deadline but it is becoming really hard for us to reserve working hours without a minimal coordination of BVCL review plan (monthly bimonthly or whatever you want).

[lou-k]

It appears that the line:

void DataTransformer<Dtype>::Transform(const cv::Mat& cv_img

is no longer surrounded by a #ifndef OSX in the dev branch. Does that mean this change will work in OSX now?

@mtamburrano can you rebase this regardless? The PR currently breaks the build with:

src/caffe/data_transformer.cpp:195:2: error: #endif without #if
#endif
 ^
src/caffe/data_transformer.cpp:197:2: error: unterminated conditional directive
#ifndef OSX

[shelhamer]

I merged this with a little grooming of my own in 02d9170. I hope this is useful, as it has been requested, but I'm not entirely excited myself for the creeping dependence on OpenCV. While it is helpful in some environments, it is a heavy dependency in others, so I plan to have it split off according to #1738 eventually.