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

Centralized validation via setters #7218

Merged
merged 11 commits into from Mar 13, 2019

Conversation

@rnett
Copy link
Contributor

commented Feb 22, 2019

What changes were proposed in this pull request?

Moved array and non-negative validation to ValidationUtils
Made setters use those validation methods.
Made all setting methods go through setters.

How was this patch tested?

No changes to implementations. It compiles.

Quick checklist

The following checklist helps ensure your PR is complete:

  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
rnett added 2 commits Feb 22, 2019
Moved validation of arrays and non-negativeness to ValidationUtils.
Made setters use those validation methods.
Made all setting methods go through setters.

@saudet saudet requested a review from AlexDBlack Feb 22, 2019

@AlexDBlack
Copy link
Contributor

left a comment

Just an initial review on ValidationUtils.
As for the consolidation of builder validation: I'll review that separately later (that'll take a little longer to check + think about).

cleaned up imports, used correct Preconditions class
Used Preconditions string formatting correctly
Add no-arg constructor to ValidationUtils
@AlexDBlack
Copy link
Contributor

left a comment

Overall there's some good ideas and cleanup here. 👍

One change that I'm a little hesitant to accept is the change to using setters in every builder method. When there is additional validation in the setter, that makes sense. But in most cases, there isn't additional validation - and thus the change from public builder x(Object x){ this.x = x to public builder x(Object x){ this.setX(x) has no benefit.
I'm hesitant to accept changes when something isn't broken, if the new version doesn't have any benefit. There's an argument to be made for consistency, but there's also the argument that it makes the code slightly harder to follow due to the additional indirection/method call (that does exactly what the code already does - just sets the field).

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

The main reason I changed everything to setters is that if someone goes back and adds or changes validation for a parameter, they don't have to change it then.

It also makes it easier when calling those methods to just always use the setter, rather than having to look at the setter and see if there is any validation there or not for each use.

@AlexDBlack
Copy link
Contributor

left a comment

The main reason I changed everything to setters is that if someone goes back and adds or changes validation for a parameter, they don't have to change it then.

That seems like a reasonable justification to do this, even though it's somewhat unconventional.

Overall I'm happy with this PR given the recent fixes/updates - I'll run some tests locally, and assuming they pass we can look at merging this.

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

OK, so test results: looks like we have a small nuber of test failures: introduced by this PR
image

EDIT: a few more:
image

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I will finish fixing this soonish, its just end of quarter crunch time atm and I haven't had much time to look at it.

I'm not sure whats up with the embedding layer, I didn't change anything there are barely anything else in Dense or Feedforeward.

Quite a few of the errors come from 1D CNN, which may have been from passing null to a vararg method, or Upsampling1D, where I forgot a return.

The TestInvalidConfiguration.testSubsamplingInvalidPadding error comes from expecting the wrong exception (the -1 is now caught in the setter using Preconditions).

testSubsamplingInvalidKernel2 and testSubsamplingInvalidPadding2 are now valid. When setting the padding to a single value that value is automatically duplicated out to 2, as was done in some other layers. I can remove these, but wanted to check before I start deleting tests.

test1dForward was because one of the methods was named setInputSize and conflicted with my setter.

This should fix most of the errors, but I don't have tests running yet so I can't check (I will within a couple weeks).

@AlexDBlack
Copy link
Contributor

left a comment

I pushed up test fixes and some final tweaks. I figured given it was already really close I'd finish it off and get it merged sooner rather than later.
Thanks again for the PR!

@AlexDBlack AlexDBlack merged commit 2bc1484 into eclipse:master Mar 13, 2019

0 of 3 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
continuous-integration/jenkins/pr-head This commit cannot be built
Details
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.