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

Issues with truncated polynomial order in LeastSquaresFit and LeastSquaresFitHistory #13498

Closed
bwspenc opened this issue May 30, 2019 · 2 comments · Fixed by #13510
Closed

Issues with truncated polynomial order in LeastSquaresFit and LeastSquaresFitHistory #13498

bwspenc opened this issue May 30, 2019 · 2 comments · Fixed by #13510
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.

Comments

@bwspenc
Copy link
Contributor

bwspenc commented May 30, 2019

Bug Description

The LeastSquaresFit and LeastSquaresFitHistory vectorpostprocessors are both using an option in PolynomialFit (which does the heavy lifting) that automatically truncates the order of the polynomial if an insufficient number of points is provided.

There are a few issues with this:

  1. The automatic truncation code requires an arbitrary excessively high number of terms:
      _order = (_x.size() / 10) + 1;

This choice seems totally insane! @andrsd: git blame says you last touched this code. Do you know anything about that?

  1. If you don't use the truncation option, there's an error check, but the logic for it is wrong. The < comparison operation should be a <=.

  2. If you hit the error, rather than calling mooseError(), it throws a std::domain_error, which doesn't get handled, so the error message never gets printed out.

We need to fix all of these issues. I'm going to provide an option on whether to truncate the polynomial order in those vpps, and add a thorough set of test cases to make sure this is working correctly.

Steps to Reproduce

Use LeastSquaresFitHistory with 3 data points and ask for a 2nd order polynomial. You only get a 1st order polynomial.

Impact

We use this pretty heavily in Grizzly. I'm afraid that we may be inadvertently having the polynomial order truncated in problems we care about.

@bwspenc bwspenc added the T: defect An anomaly, which is anything that deviates from expectations. label May 30, 2019
@andrsd
Copy link
Contributor

andrsd commented May 31, 2019

git blame says you last touched this code. Do you know anything about that?

I did not write this code. The change you see in git history is from the old SVN days. The date tells me that it was when we rolled out the new MOOSE (what we internally call MOOSE 2.0). We probably moved that file from somewhere else and because git-svn did not handle moves properly it looks like a new file that I wrote.

The true author of this line is @permcody. This change occurred in May 2009, 21f90a9c555a7a9a2226d992a3c466d914f347f4 in git history.

@permcody
Copy link
Member

permcody commented May 31, 2019

OK - so 2009... A lot has changed since then 😄

  1. The automatic truncation code requires an arbitrary excessively high number of terms:
    _order = (_x.size() / 10) + 1;
    This choice seems totally insane! @andrsd: git blame says you last touched this code. Do you know anything about that?

Clearly a hack for the single problem I was running at the time. This is long before I was properly in the framework and utility mindset. It does appear arbitrary so feel free to improve it.

  1. If you don't use the truncation option, there's an error check, but the logic for it is wrong. The < comparison operation should be a <=.

Yep

  1. If you hit the error, rather than calling mooseError(), it throws a std::domain_error, which doesn't get handled, so the error message never gets printed out.

Now this isn't necessarily a bad choice and we do this for other pure utilities that have zero dependence on MOOSE. It just passes the responsibility to the MOOSE class that calls the utility to handle the error. To call MooseError you'll have to add the MooseError.h header. Also you may want to provide a more informative error about input file parameters or other things more relevant to why the error was produced, which you can't do as easily at this low level.

@permcody permcody added C: Framework T: task An enhancement to the software. P: normal A defect affecting operation with a low possibility of significantly affects. and removed T: task An enhancement to the software. labels May 31, 2019
bwspenc added a commit to bwspenc/moose that referenced this issue May 31, 2019
bwspenc added a commit to bwspenc/moose that referenced this issue Jun 3, 2019
…oses idaholab#13498

Add a 'truncate_order' option to LeastSquaresFit and LeastSquaresFitHistory

Correct logic for determining required points for a given polynomial
order in PolynomialFit.C

Add tests for fits of polynomials with order 0, 1, and 2

Add test for insufficient order error
bwspenc added a commit to bwspenc/moose that referenced this issue Jun 3, 2019
…oses idaholab#13498

Add a 'truncate_order' option to LeastSquaresFit and LeastSquaresFitHistory

Correct logic for determining required points for a given polynomial
order in PolynomialFit.C

Add tests for fits of polynomials with order 0, 1, and 2

Add test for insufficient order error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants