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

Hdf5 output layer #252

Merged
merged 5 commits into from
Mar 24, 2014
Merged

Hdf5 output layer #252

merged 5 commits into from
Mar 24, 2014

Conversation

kloudkl
Copy link
Contributor

@kloudkl kloudkl commented Mar 23, 2014

This PR resolves #213 and also closes #220.

@sergeyk, please review and check whether this is what you asked for.

@sergeyk
Copy link
Contributor

sergeyk commented Mar 23, 2014

Looks great! One thing, could you make sure that the Forward/Backward GPU mode functions are split into separe .cu file, like in the other layers? Will merge as soon as that's done.

@@ -142,4 +142,30 @@ void hdf5_load_nd_dataset_helper(
file_id, dataset_name_, blob->mutable_cpu_data());
}

template <>
void hdf5_save_nd_dataset<float>(
const hid_t file_id, const string dataset_name, const Blob<float>& blob) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kloudkl one probably want to save multiple blobs in the same HDF5 file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation assumes that a dataset corresponds to a blob as hdf5_load_nd_dataset indicates. Future needs would be satisfied in some future PRs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, hdf5_load_nd_dataset can load different Blobs from the same file, but it's true it could be a future feature.

@kloudkl
Copy link
Contributor Author

kloudkl commented Mar 24, 2014

The ImagesLayer.cpp and WindowsDataLayer.cpp need to be split in another PR.

@sergeyk
Copy link
Contributor

sergeyk commented Mar 24, 2014

Agree with @kloudkl on both counts. Is a commit to split to .cu coming up?

@shelhamer
Copy link
Member

This introduces a new layer and proto field so it needs to be coordinated with @jeffdonahue's #219.

@kloudkl
Copy link
Contributor Author

kloudkl commented Mar 24, 2014

The ImagesLayer.cpp and WindowsDataLayer.cpp are coming up.

@sguada
Copy link
Contributor

sguada commented Mar 24, 2014

@kloudkl I think ImagesLayer.cpp and WindowsDataLayer.cpp should go in a different PR. Leave this one only for HDF5Output

@kloudkl
Copy link
Contributor Author

kloudkl commented Mar 24, 2014

Yes, they are in #253.

@sergeyk
Copy link
Contributor

sergeyk commented Mar 24, 2014

@shelhamer let's avoid blocking situations as agreed. let's merge this, then 219, then fix whatever needs fixin'

@shelhamer
Copy link
Member

@sergeyk agreed–just flagging for Jeff so it isn't missed.

@kloudkl
Copy link
Contributor Author

kloudkl commented Mar 24, 2014

The changes to src/caffe/proto/caffe.proto are inline with #219.

 +  
 +  optional HDF5OutputParameter hdf5_output_param = 1001;
 +}
 +
 +message HDF5OutputParameter {
 +  optional string file_name = 1;

But I am afraid that adding enum LayerType { HDF5_OUTPUT = 100; } to message LayerParameter would cause merge conflicts with #219.

@shelhamer
Copy link
Member

@kloudkl don't worry about it. As Sergey said we'll merge this, and having made the needed notes, #219 can be updated and merged next.

sergeyk added a commit that referenced this pull request Mar 24, 2014
@sergeyk sergeyk merged commit d3e4c21 into BVLC:dev Mar 24, 2014
@sergeyk
Copy link
Contributor

sergeyk commented Mar 24, 2014

Thanks @kloudkl

@kloudkl kloudkl deleted the hdf5_output_layer branch March 24, 2014 11:53
@shelhamer shelhamer mentioned this pull request May 20, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
thatguymike pushed a commit to thatguymike/caffe that referenced this pull request Nov 17, 2016
Add support for GPU transforms to reduce CPU load
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.

4 participants