Skip to content

Constraints - 1 #215

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

Merged
merged 11 commits into from
Feb 27, 2021
Merged

Constraints - 1 #215

merged 11 commits into from
Feb 27, 2021

Conversation

JimClarke5
Copy link
Contributor

Adding Constraints.
Constraints impose constraints on weight values.

This does not include the RadialConstraint as that requires while operations.

This should be independent class that do not have dependencies on other framework packages.

*
* @param <T> the date type for the weights
*/
public abstract class Constraint<T extends TNumber> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does an instance of Constraint need to be specialized to a particular tensor type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 done

* @return the element-wise square root.
*/
protected Operand<T> sqrt(Operand<T> x) {
Class<T> type = x.type();
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify x not null as we do in clip?

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
Contributor

Choose a reason for hiding this comment

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

👍 done

private final int[] axes;

/**
* Create a MaxNorm constraint using {@link #MIN_VALUE_DEFAULT} for the min value, {@link
Copy link
Contributor

Choose a reason for hiding this comment

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

MaxNorm -> MinMaxNorm in all constructor header comments

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
Contributor

Choose a reason for hiding this comment

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

👍 done

weights,
tf.math.add(
cast(tf, tf.constant(EPSILON), type),
sqrt(
Copy link
Contributor

Choose a reason for hiding this comment

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

This computation of the norms keeps coming up. Perhaps add a superclass method for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 done

ReduceSum.keepDims(Boolean.TRUE)))));
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

We have three instances of this axis-management so far. I could imagine adding a superclass like AbstractNormConstraint to handle it. (But I could go either way.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, there are only 3 Constraint classes that use it. I don't think another class is warranted.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

for (AtomicInteger i = new AtomicInteger();
i.get() < testValues.length;
i.getAndIncrement()) {
MinMaxNorm<TFloat32> instance =
Copy link
Contributor

Choose a reason for hiding this comment

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

From this point forward, the test is erroneously a copy of MinMaxNormTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I'll fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 done

for (AtomicInteger i = new AtomicInteger();
i.get() < testValues.length;
i.getAndIncrement()) {
MaxNorm<TFloat32> instance = new MaxNorm<>(tf, testValues[i.get()]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Erroneously a copy of MaxNormTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I'll fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 done

return result;
}

/** Test of call method, of class MaxNorm. */
Copy link
Contributor

Choose a reason for hiding this comment

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

MaxNorm -> UnitNorm (not just in the comment, but in the code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I'll fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 done

Add norm method on Constraint that is leveraged by the xxxxNorm constraints.
Fix unit test cases to properly test the actual classes (oops).
Fix Javadoc
Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

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

All right this one is ready to to, thanks @JimClarke5 and thanks @deansher for reviewing it!

@karllessard karllessard merged commit 5532bf0 into tensorflow:master Feb 27, 2021
@JimClarke5 JimClarke5 deleted the Constraints1 branch May 2, 2021 15:10
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.

3 participants