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

Add Upsample Layer #2255

Merged
merged 19 commits into from
Jun 28, 2024
Merged

Add Upsample Layer #2255

merged 19 commits into from
Jun 28, 2024

Conversation

fiedorowicz1
Copy link
Contributor

@fiedorowicz1 fiedorowicz1 commented Apr 27, 2023

Adds a distconv supported upsampling layer. Currently, this layer can only perform nearest neighbor upsampling and only works on NVIDIA machines.

@fiedorowicz1 fiedorowicz1 marked this pull request as ready for review June 19, 2024 16:40
Copy link
Collaborator

@benson31 benson31 left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good. Just a couple of small things.

include/lbann/layers/transform/upsample.hpp Outdated Show resolved Hide resolved
include/lbann/layers/transform/upsample.hpp Outdated Show resolved Hide resolved
Comment on lines 31 to 38
template <typename TensorDataType, data_layout Layout, El::Device Device>
template <typename ArchiveT>
void upsample_layer<TensorDataType, Layout, Device>::serialize(ArchiveT& ar)
{
using DataTypeLayer = data_type_layer<TensorDataType>;
ar(::cereal::make_nvp("DataTypeLayer",
::cereal::base_class<DataTypeLayer>(this)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this save more of the state? At least the upscale mode and scaling factors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/layers/transform/upsample.cpp Outdated Show resolved Hide resolved
src/layers/transform/upsample.cpp Outdated Show resolved Hide resolved

/// Pooling forward propagation with im2col
template <typename TensorDataType, data_layout Layout, El::Device Dev>
void upsample_layer<TensorDataType, Layout, Dev>::fp_compute_im2col()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might consider just deleting this and the corresponding bp_ function to just remove unnecessary code. The call-sites are commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


/// Pooling forward propagation with im2col
template <typename TensorDataType, data_layout Layout, El::Device Dev>
void upsample_layer<TensorDataType, Layout, Dev>::bp_compute_im2col()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@benson31 benson31 left a comment

Choose a reason for hiding this comment

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

A few suggestions for further tidying.

#include "lbann/utils/dnn_lib/upsample.hpp"
#endif // LBANN_HAS_DNN_LIB
#include "lbann/utils/exception.hpp"
#include "lbann/utils/im2col.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#include "lbann/utils/im2col.hpp"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 270 to 275
/// Pooling forward propagation with im2col
void fp_compute_im2col();

/// Pooling forward propagation with im2col
void bp_compute_im2col();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Pooling forward propagation with im2col
void fp_compute_im2col();
/// Pooling forward propagation with im2col
void bp_compute_im2col();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 264 to 267
/// Pooling forward propagation with DNN library
void fp_compute_dnn();

/// Pooling backward propagation with DNN library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Pooling forward propagation with DNN library
void fp_compute_dnn();
/// Pooling backward propagation with DNN library
/// Upscaling forward propagation with DNN library
void fp_compute_dnn();
/// Upscaling backward propagation with DNN library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

ci_test/unit_tests/test_unit_layer_upsample.py Outdated Show resolved Hide resolved
ci_test/unit_tests/test_unit_layer_upsample_distconv.py Outdated Show resolved Hide resolved
include/lbann/layers/transform/upsample.hpp Outdated Show resolved Hide resolved
include/lbann/layers/transform/upsample.hpp Outdated Show resolved Hide resolved
include/lbann/layers/transform/upsample.hpp Show resolved Hide resolved
@fiedorowicz1 fiedorowicz1 requested a review from tbennun June 25, 2024 18:13
@bvanessen bvanessen merged commit c920d33 into LLNL:develop Jun 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants