This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Generalized reshape_like operator #11928
Merged
szha
merged 5 commits into
apache:master
from
sbodenstein:feature/GeneralizedReshapeLike
Aug 11, 2018
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,6 +476,31 @@ void HardSigmoidBackward(const nnvm::NodeAttrs& attrs, | |
}); | ||
} | ||
|
||
struct ReshapeLikeParam : public dmlc::Parameter<ReshapeLikeParam> { | ||
int lhs_begin, rhs_begin; | ||
dmlc::optional<int> lhs_end, rhs_end; | ||
DMLC_DECLARE_PARAMETER(ReshapeLikeParam) { | ||
DMLC_DECLARE_FIELD(lhs_begin).set_default(0).describe( | ||
"Defaults to 0. " | ||
"The beginning index along which the lhs dimensions are to be " | ||
"reshaped. Supports negative indices."); | ||
DMLC_DECLARE_FIELD(lhs_end) | ||
.set_default(dmlc::optional<int>()) | ||
.describe("Defaults to None. " | ||
"The ending index along which the lhs dimensions are to be " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This describe comment seems to have a spurious sentence fragment in it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
"used for reshaping. Supports negative indices."); | ||
DMLC_DECLARE_FIELD(rhs_begin).set_default(0).describe( | ||
"Defaults to 0. " | ||
"The beginning index along which the rhs dimensions are to be used for " | ||
"reshaping. Supports negative indices."); | ||
DMLC_DECLARE_FIELD(rhs_end) | ||
.set_default(dmlc::optional<int>()) | ||
.describe("Defaults to None. " | ||
"The ending index along which the rhs dimensions are to be " | ||
"used for reshaping. Supports negative indices."); | ||
} | ||
}; | ||
|
||
/*! \brief Unary compute */ | ||
#define MXNET_OPERATOR_REGISTER_UNARY(__name$) \ | ||
NNVM_REGISTER_OP(__name$) \ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if these two parameters should be made optional too, given that reshape_like op has been around for 10 months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are optional in the sense that they have default values
DMLC_DECLARE_FIELD(lhs_begin).set_default(0)
that match the old behaviour if not explicitly specified (so no backward-compatibility is broken). I thoughtdmlc::optional<int>
was simply for the case where you had to handle theNone
-value case, which is not necessary to support forlhs_begin
andrhs_begin
. Note that the same is done forslice_axis
,begin
is anint
andend
isdmlc::optional<int>
.Or am I misunderstanding something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the case where the parameters may be in some serialized format, it may be necessary to support null values to ensure compatibility there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if its necessary, I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szha Can I clarify something? Maybe I am misunderstanding. Are you saying that it is required to make new parameters on existing layers optional<...> for backward compatibility reasons, even if they have defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taliesinb I think it doesn't hurt in this case and was suggesting that we lean on the safer side. There might be cases where the default value filling on the frontend fails, such as deserializing a graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szha ok. so it sounds like there isn't a particular policy at the moment about how to add new parameters to existing layers. do you mind if i ask on the mailing list about what such a policy should be, just for the next time this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course not. good idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szha: I've changed
lhs_begin
andrhs_begin
to use optional (and added tests for this case). Can we merge?