idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.77k stars 1.05k forks source link

Coupleable with scalar & field variables #14667

Open snschune opened 4 years ago

snschune commented 4 years ago

Bug Description

@permcody

If you access a vector of coupled variables like this

unsigned int nc = coupledComponents("name");
for (unsigned int j = 0; j < nc; ++j)
  _var[j] = coupledValue("name", j);

but you couple only scalar variables in the input file, no error occurs because coupledComponents returns zero in this case. This is the desired behavior because Coupleable allows coupling both field and scalar variables.

In the above case, the developer expected that field variables are coupled. However, if the user couples scalar variables (which Coupleable supports), then coupledComponents("name") returns zero. There is a check on scalar variables with proper error message in coupledValue but that never gets called. This makes it impossible for developers to properly check consistency of the user input.

Another question I have: it appears Coupleable allows to mix scalar and field variable in a single coupled parameter vector. How do you know in a class you develop which one is which without calling lower level function? How do you know how many do you have of each kind?

andrsd commented 4 years ago

Check coupledScalarComponents in ScalarCoupleable.h - it does what coupledComponents does but for scalar variables.

But, you are right there could be possibly more error checking - I saw people making the same mistake, i.e. using coupledComponents for scalar variables...

snschune commented 4 years ago

What I don't understand is why Coupleable allows scalar variables to be coupled here?

Shouldn't we simply error out under this condition?

andrsd commented 4 years ago

That might be a left over from refactoring ScalarCoupleable out of Coupleable...

snschune commented 4 years ago

I want Coupleable to support mixed defaults and variables, currently is does only one and the other. If I find the time to do that, I can look at removing scalar variables out of Coupleable as well. Would that be the right thing to do?

permcody commented 4 years ago

You mean combine the coupleable and scalarcouplable interfaces into one? That's actually the way it used to be and I liked it (old man voice).

On Thu, Jan 30, 2020 at 3:37 PM Sebastian Schunert notifications@github.com wrote:

I want Coupleable to support mixed defaults and variables, currently is does only one and the other. If I find the time to do that, I can look at removing scalar variables out of Coupleable as well. Would that be the right thing to do?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/14667?email_source=notifications&email_token=AAXFOIE5UIO52FF7SEAX7STRANJDNA5CNFSM4KNLPX72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKM2OVY#issuecomment-580495191, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXFOIBTBXGEJCPLBG7QGB3RANJDNANCNFSM4KNLPX7Q .

andrsd commented 4 years ago

If I find the time to do that, I can look at removing scalar variables out of Coupleable as well. Would that be the right thing to do?

IMO if scalar-anything gets into Coupleable, it should be an error. And if field-anything gets into ScalarCoupleable it should be an error as well.