This repository has been archived by the owner. It is now read-only.

Add ConditionalReplaceValueTransformWithDefault #409

Merged
merged 6 commits into from Sep 3, 2017

Conversation

Projects
None yet
2 participants
@huitseeker
Copy link
Contributor

huitseeker commented Aug 28, 2017

A compact version of partitioning the replacement values for a column by a condition,
offshoot of work on the intrusion demo.

huitseeker added some commits Aug 28, 2017

@huitseeker huitseeker requested review from AlexDBlack , kepricon and turambar Aug 29, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

If condition is true, replace with 'newVal'.
If condition is false, replace with 'defaultValue'??? (Which means - builder javadoc is very wrong, and the class javadoc doesn't explain the condition false case).

Assuming my interpretation is correct, I'm not sure "default" is the best name for this. It's one value if true, another if false - I don't see why the false case value should be called "default".

@huitseeker

This comment has been minimized.

Copy link
Contributor

huitseeker commented Aug 31, 2017

@AlexDBlack Indeed. I hope my latest fixes all that.

To be clear, this operation is strictly better than two successive conditional replaces, not just form performance, but

  • in the case where we wish to have a non-empty intersection between the image of one side of the partition and the domain of the other (e.g. if x <= 0 replace by 1 else replace by 0)
  • and it makes the case where the condition is a pain to negate, such as regexes, a bit easier (than the use of Boolean condition operators).

@huitseeker huitseeker requested a review from AlexDBlack Aug 31, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Fine except for the builder javadoc.

* @param defaultVal Value to use as replacement, if condition is satisfied
* @param condition Condition that must be satisfied for replacement
*/
public Builder conditionalReplaceValueTransformWithDefault(String column, Writable newVal, Writable defaultVal, Condition condition) {

This comment has been minimized.

@AlexDBlack

AlexDBlack Sep 1, 2017

Member

Should align with the yesVal and noVal semantics of the constructor.

@@ -1327,6 +1328,19 @@ public Builder conditionalReplaceValueTransform(String column, Writable newValue
}

/**
* Replace the values in a specified column with a specified new value, if some condition holds.
* If the condition does not hold, the original values are not modified.

This comment has been minimized.

@AlexDBlack

AlexDBlack Sep 1, 2017

Member

Still incorrect, does not match class javadoc.

*
* @param column Column to operate on
* @param newVal Value to use as replacement, if condition is satisfied
* @param defaultVal Value to use as replacement, if condition is satisfied

This comment has been minimized.

@AlexDBlack

AlexDBlack Sep 1, 2017

Member

is not satisfied

@huitseeker

This comment has been minimized.

Copy link
Contributor

huitseeker commented Sep 1, 2017

Ouch, apologies: I messed up, I should have paid more attention to this.

@AlexDBlack AlexDBlack merged commit 2d96a88 into deeplearning4j:master Sep 3, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.