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

Range checking for vector parameters #3989

Merged
merged 6 commits into from Oct 7, 2014
Merged

Conversation

dschwen
Copy link
Member

@dschwen dschwen commented Oct 3, 2014

Comprehensive range (and length) checking for vector parameters (closes #3988).

@moosebuild
Copy link
Contributor

Results of testing ee84d05 using moose_PR_pre_check recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6201

@moosebuild
Copy link
Contributor

Results of testing ee84d05 using moose_PR_test_dbg recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6203

@moosebuild
Copy link
Contributor

Results of testing ee84d05 using moose_PR_test recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6202

@permcody
Copy link
Member

permcody commented Oct 3, 2014

Nicely done, I wonder if we should pull the range checking stuff from the parser completely. We don't have to do that here, just tossing that out there. With this PR we are doing checks for vector stuff only in InputParameters, but we are doing checks for scalars in both places. It's just inconsistent.

@dschwen
Copy link
Member Author

dschwen commented Oct 3, 2014

Yeah, I forgot about the other place. Why is the stuff checked twice?
On Oct 3, 2014 11:11 AM, "Cody Permann" notifications@github.com wrote:

Nicely done, I wonder if we should pull the range checking stuff from the
parser completely. We don't have to do that here, just tossing that out
there. With this PR we are doing checks for vector stuff only in
InputParameters, but we are doing checks for scalars in both places. It's
just inconsistent.


Reply to this email directly or view it on GitHub
#3989 (comment).

@permcody
Copy link
Member

permcody commented Oct 3, 2014

There's actually not a really good reason. I added it to the parser first, because it was natural to do it while you're filling in the the parameters. You've already dynamic casted everything and you're sitting there with the parameters in hand. The problem arises when people skip using the parser and just build an object programmatically. I went back and added code to do it later in the InputParameters class during a manual sanity check. I suppose that's good enough... Just didn't want to delete all my precious code out of the Parser but we probably should.

@moosebuild
Copy link
Contributor

Results of testing 135b357 using moose_PR_pre_check recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6213

@moosebuild
Copy link
Contributor

Results of testing 135b357 using moose_PR_test recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6214

@moosebuild
Copy link
Contributor

Results of testing 135b357 using moose_PR_test_dbg recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6215

@dschwen
Copy link
Member Author

dschwen commented Oct 4, 2014

Added the tests for other types (I meant to do that at least for int, but messed up the material params). And voila, the test for the long type is not executed, so the tests for this PR will now fail. Why?

@permcody
Copy link
Member

permcody commented Oct 4, 2014

Is this a pop quiz? Where is the change set?

@dschwen
Copy link
Member Author

dschwen commented Oct 4, 2014

Oops forgot to push. Am in tetonia now.
On Oct 4, 2014 11:03 AM, "Cody Permann" notifications@github.com wrote:

Is this a pop quiz? Where is the change set?


Reply to this email directly or view it on GitHub
#3989 (comment).

@permcody
Copy link
Member

permcody commented Oct 4, 2014

You can't be in Tetonia. In the time it took you to type that message you would have already passed through. :)

@moosebuild
Copy link
Contributor

Results of testing cbc3e17 using moose_PR_pre_check recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6216

@moosebuild
Copy link
Contributor

Results of testing cbc3e17 using moose_PR_test recipe:

Failed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6217

@moosebuild
Copy link
Contributor

Results of testing cbc3e17 using moose_PR_test_dbg recipe:

Failed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6218

@dschwen
Copy link
Member Author

dschwen commented Oct 6, 2014

The problem seems to be that the vector is read in with size 0! (which coincidentally skips range checking entirely... that is another bug)

Also, there are no uses of a std::vector<long> in MOOSE at all, so this may be broken.

@dschwen
Copy link
Member Author

dschwen commented Oct 6, 2014

Uhm, yeah, vectors of long are not implemented. I'm adding them in the next commit :-/

@permcody
Copy link
Member

permcody commented Oct 6, 2014

That means that nobody has ever needed them. Most vector parameters are Real values in MOOSE.

@dschwen
Copy link
Member Author

dschwen commented Oct 6, 2014

Sure, but adding them costs a single line (that could have saved me quite some confusion).

@moosebuild
Copy link
Contributor

Results of testing 0c66414 using moose_PR_pre_check recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6225

@moosebuild
Copy link
Contributor

Results of testing 0c66414 using moose_PR_test recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6226

@moosebuild
Copy link
Contributor

Results of testing 0c66414 using moose_PR_test_dbg recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6227

@dschwen
Copy link
Member Author

dschwen commented Oct 6, 2014

Please don't merge yet

@moosebuild
Copy link
Contributor

Results of testing 041ef39 using moose_PR_pre_check recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6228

@moosebuild
Copy link
Contributor

Results of testing 041ef39 using moose_PR_test recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6229

@moosebuild
Copy link
Contributor

Results of testing 041ef39 using moose_PR_test_dbg recipe:

Passed on: linux-gnu

View the results here: https://www.moosebuild.com/view_job/6230

@dschwen
Copy link
Member Author

dschwen commented Oct 6, 2014

I wonder if I should make the dimensions of the simulation domain available in the check expression. LIBMESH_DIM seems to be a compiled in maximum dimension, is there a standard way to get the problem dimension? (MooseMesh::dimension() maybe?)

And is this actually desired?

@permcody
Copy link
Member

permcody commented Oct 7, 2014

If the inventor can't think of a use case for his thingy, maybe we don't need it (even if it is cool)?

@dschwen
Copy link
Member Author

dschwen commented Oct 7, 2014

I'm not questioning the usefulness for the range checking at all, I already have use cases. I'm just wondering if I should make the dimension available to allow for vector length checks against problem dimension. I.e. a point needs at least dim entries (or do we have Point type parameters that can already do that?)

@permcody
Copy link
Member

permcody commented Oct 7, 2014

How do you know if it has "dim" number of entries? All Points,
RealVectors, etc. have LIBMESH_DIM entries.

On Monday, October 6, 2014, Daniel Schwen notifications@github.com wrote:

I'm not questioning the usefulness for the range checking at all, I
already have use cases. I'm just wondering if I should make the dimension
available to allow for vector length checks against problem dimension. I.e.
a point needs at least dim entries (or do we have Point type parameters
that can already do that?)


Reply to this email directly or view it on GitHub
#3989 (comment).

@dschwen
Copy link
Member Author

dschwen commented Oct 7, 2014

We have cases, where any number of entries is accepted and all missing entries up to the third (hardcoded of course) are zero padded. I'm not terribly excited by that.

@permcody
Copy link
Member

permcody commented Oct 7, 2014

My point is that this check happens on a Point object, not on the
information in the file (if it comes from a file). It will always have
LIBMESH_DIM entries so you don't really know how they got populated.

On Tue, Oct 7, 2014 at 7:18 AM, Daniel Schwen notifications@github.com
wrote:

We have cases, where any number of entries is accepted and all missing
entries up to the third (hardcoded of course) are zero padded. I'm not
terribly excited by that.


Reply to this email directly or view it on GitHub
#3989 (comment).

permcody added a commit that referenced this pull request Oct 7, 2014
Range checking for vector parameters
@permcody permcody merged commit 21d4ea4 into idaholab:devel Oct 7, 2014
@dschwen dschwen deleted the vecrange_3988 branch November 10, 2014 21:00
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.

None yet

3 participants