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

check block-restriction in duplicateVariableCheck #28570

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

jmeier
Copy link
Contributor

@jmeier jmeier commented Sep 8, 2024

Currently, in FEProblemBase::duplicateVariableCheck Moose checks that order and family is matching. In the same function, Moose does no checks on the block-restriction. However, it would be very good if Moose would also check the block restriction.

Furthermore, the error message on family and order should be more verbose and give more information.

closes #28568

@jmeier
Copy link
Contributor Author

jmeier commented Sep 8, 2024

Dear reviewers,

This PR aims at improving the error message for the variable type, but shows in an error for the variable type e.g. MONOMIAL of order 0: The 'order' is only displayed as an integer value. Should a ‘better’ value be displayed there? If so, how?

Kind regards,
Jörg

@jmeier
Copy link
Contributor Author

jmeier commented Sep 8, 2024

Dear @GiudGiud,

you mentioned in #28560 (comment) that other aspects of the variables also should be checked. Therefore, I introduced the parameter variable_deduplication_check as MultiMooseEnum. Extending (and controlling) checks should be easier this way.

Jörg

@jmeier
Copy link
Contributor Author

jmeier commented Sep 8, 2024

So... The new check on the block-restriction makes several tests fail. I see three ways on how to deal with this:

  1. Add cli_args = 'Problem/variable_deduplication_check=ALLOW_MISMATCHING_BLOCK_RESTRICTIONS' and allow_warnings = true to all tests

  2. We introduce IGNORE_MISMATCHING_BLOCK_RESTRICTIONS (what shows no warning for mismatching blocks) and add cli_args = 'Problem/variable_deduplication_check=IGNORE_MISMATCHING_BLOCK_RESTRICTIONS' to all tests

  3. We change the tests. That would be very time-consuming.

I suggest option 2. What do you think?

@moosebuild
Copy link
Contributor

moosebuild commented Sep 8, 2024

Job Documentation on dcd356f wanted to post the following:

View the site here

This comment will be updated on new commits.

@GiudGiud GiudGiud self-assigned this Sep 8, 2024
@jmeier
Copy link
Contributor Author

jmeier commented Sep 9, 2024

jmeier requested review from bwspenc, dschwen, jiangwen84, cpgr and recuero as code owners

Oh no! Sorry for pestering all of you! I had to change some tests due to the now stricter testing on the block restriction. Most of the tests had to be changed due to the handling of the block-restriction in [Physics/SolidMechanics/QuasiStatic] (e.g. also see #28308).

@jmeier
Copy link
Contributor Author

jmeier commented Sep 11, 2024

Dear @GiudGiud, if I interpred the logs correctly, the tests for Moose are all green now. There are failing tests for Mastodon and Raccoon. I've a fix for Mastodon/examples/ex05a.ex05a but not for the others where I'd need help. How do we proceed?

@GiudGiud
Copy link
Contributor

I ll take a look. We're working on releasing fluid properties right now so there's quite a few apps in the red in the meantime

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

works for me.
It catches the lower D block restriction issues when the proper block restriction has been passed to one of the variables outside the action, so it helps there too

@GiudGiud
Copy link
Contributor

GiudGiud commented Sep 12, 2024

Will need a patch for

  • BISON
  • subchannel
  • mastodon
  • malamute
    I ll work on these

@moosebuild
Copy link
Contributor

moosebuild commented Sep 12, 2024

Job Coverage on dcd356f wanted to post the following:

Framework coverage

e494f1 #28570 dcd356
Total Total +/- New
Rate 85.04% 85.04% +0.00% 82.00%
Hits 105669 105710 +41 41
Misses 18590 18594 +4 9

Diff coverage report

Full coverage report

Modules coverage

Solid mechanics

e494f1 #28570 dcd356
Total Total +/- New
Rate 84.90% 84.91% +0.00% 100.00%
Hits 28156 28158 +2 4
Misses 5006 5006 - 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 82.00% is less than the suggested 90.0%

This comment will be updated on new commits.

@GiudGiud
Copy link
Contributor

GiudGiud commented Sep 12, 2024

Ok looking at bison, it s breaking on adding variables with a SMALLER block restriction than the existing one (for example ANY_BLOCK_ID).

We should actually just allow that. It does not hurt to be trying to add variables that are already defined on a larger set of blocks.
Let s see how many app patches we need after that

@jmeier
Copy link
Contributor Author

jmeier commented Sep 13, 2024

Ok looking at bison, it s breaking on adding variables with a SMALLER block restriction than the existing one (for example ANY_BLOCK_ID).
We should actually just allow that. It does not hurt to be trying to add variables that are already defined on a larger set of blocks. Let s see how many app patches we need after that

When I initially created the code, I also had this thought: If we allow this, the order of commands in a Moose input file has an influence on whether this error occurs or not. Should we still allow this?

@GiudGiud
Copy link
Contributor

The only case where this happens is when you have two actions that act on the same task, adding variables. I can see this help combining actions

@jmeier
Copy link
Contributor Author

jmeier commented Sep 13, 2024

We should actually just allow that. It does not hurt to be trying to add variables that are already defined on a larger set of blocks. Let s see how many app patches we need after that

With the widening on the check regarding block-restriction "Bison" seems now to be green alogg all "internal app tests". There are still failing "public app tests" for:

  • Mastodon (I have a fix for Mastodon/examples/ex05a.ex05a - but not the other 3)
  • Ferret (this is new! but from the error message one could think this is unrelated)
  • Raccoon

and the "controlled app tests" are also red.

@GiudGiud
Copy link
Contributor

GiudGiud commented Sep 13, 2024

Ferret and racoon are unrelated
Controlled is subchannel, I m looking into it

EDIT:
Subchannel patch is there: https://github.inl.gov/ncrc/subchannel/pull/336

Do you mind creting the patch to mastodon and tagging me? If it s not complete let me know and I can look at the remaining test failures

@GiudGiud
Copy link
Contributor

Subchannel patch is merged

@GiudGiud
Copy link
Contributor

Mastodon patch is merged though might not show on this testing round

@GiudGiud GiudGiud merged commit f5681a9 into idaholab:next Sep 24, 2024
49 of 51 checks passed
@GiudGiud
Copy link
Contributor

Thanks for the contribution of this extra check!
That latest delay was due to next being broken since Thursday

@jmeier jmeier deleted the FEProblem_deduplicate_variable_checks branch September 24, 2024 05:19
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.

check block-restriction in FEProblemBase::duplicateVariableCheck
3 participants