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

Improve MooseObject::mooseError() with more context, correct file paths with many inputs #26945

Merged
merged 31 commits into from
Mar 23, 2024

Conversation

loganharbour
Copy link
Member

@loganharbour loganharbour commented Mar 2, 2024

Closes #26947

  • For objects created within a non-MooseObjectAction, their moose errors are associated with the syntax location for the associated action
  • For objects created in any action whose parameters are applied with applyParameters() from the action's parameters, paramError() within said objects will be linked directly to the action's parameters
  • All objects are forced to be created within the Factory or ActionFactory
  • Uses the hit node context to figure out absolute paths for all files

App patches:

@moosebuild
Copy link
Contributor

moosebuild commented Mar 2, 2024

Job Documentation on e662172 wanted to post the following:

View the site here

This comment will be updated on new commits.

@loganharbour loganharbour changed the title Associate parameters with hit nodes as much as possible and use those nodes in errors Improve MooseObject::mooseError() with more context Mar 2, 2024
@loganharbour loganharbour marked this pull request as ready for review March 2, 2024 21:15
@loganharbour loganharbour self-assigned this Mar 2, 2024
Copy link
Member

@dschwen dschwen left a comment

Choose a reason for hiding this comment

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

Otherwise looks fine

@loganharbour
Copy link
Member Author

Good again. Changes: resolved your request, added 5e9aad1. This lets you associate actions that you create in a meta action with a parameter. Easiest example is Outputs/exodus=true associates errors in the created Exodus object with Outputs/exodus=true.

@moosebuild
Copy link
Contributor

moosebuild commented Mar 3, 2024

Job Test timings on e662172 wanted to post the following:

View timings here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Mar 3, 2024

Job Coverage on e662172 wanted to post the following:

Framework coverage

354ce6 #26945 e66217
Total Total +/- New
Rate 85.31% 85.31% +0.00% 88.36%
Hits 101277 101406 +129 372
Misses 17439 17455 +16 49

Diff coverage report

Full coverage report

Modules coverage

External petsc solver

354ce6 #26945 e66217
Total Total +/- New
Rate 85.37% 85.37% - 0.00%
Hits 350 350 - 0
Misses 60 60 - 1

Diff coverage report

Full coverage report

Optimization

354ce6 #26945 e66217
Total Total +/- New
Rate 89.16% 89.17% +0.01% 100.00%
Hits 1957 1959 +2 2
Misses 238 238 - 0

Diff coverage report

Full coverage report

Peridynamics

354ce6 #26945 e66217
Total Total +/- New
Rate 77.46% 77.46% - 100.00%
Hits 3068 3068 - 1
Misses 893 893 - 0

Diff coverage report

Full coverage report

Solid mechanics

354ce6 #26945 e66217
Total Total +/- New
Rate 84.78% 84.78% -0.00% 100.00%
Hits 27113 27109 -4 2
Misses 4868 4868 - 0

Diff coverage report

Full coverage report

Stochastic tools

354ce6 #26945 e66217
Total Total +/- New
Rate 90.62% 90.62% +0.00% 100.00%
Hits 7911 7915 +4 3
Misses 819 819 - 0

Diff coverage report

Full coverage report

Thermal hydraulics

354ce6 #26945 e66217
Total Total +/- New
Rate 89.13% 89.11% -0.02% 75.00%
Hits 13972 13970 -2 3
Misses 1704 1708 +4 1

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 88.36% is less than the suggested 90.0%
  • external_petsc_solver new line coverage rate 0.00% is less than the suggested 90.0%
  • thermal_hydraulics new line coverage rate 75.00% is less than the suggested 90.0%

This comment will be updated on new commits.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

Jk, should resolve app errors

@loganharbour
Copy link
Member Author

Reminder for self: Still need to resolve file name parameters

@lindsayad
Copy link
Member

What is the confusion? It is possible that I am the confused one. Do we expect the changes here to incur app failures, and the plan is to patch them?

@loganharbour
Copy link
Member Author

The path base where to look for FileName params (files) is not working with this. Working on fixing those rules.

@loganharbour loganharbour changed the title Improve MooseObject::mooseError() with more context Improve MooseObject::mooseError() with more context, correct file paths with many inputs Mar 16, 2024
@loganharbour
Copy link
Member Author

Excuse all of the spam, I didn't plan on this needing this much more... but it is actually ready for review. few app failures, working on those fixes.

@loganharbour loganharbour force-pushed the disallow_characters branch 2 times, most recently from e6b2940 to 0423b82 Compare March 19, 2024 22:17
Copy link
Member

@permcody permcody left a comment

Choose a reason for hiding this comment

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

I don't see anything concerning here. The only thing I didn't understand as a whole is the "clone" versus "copyConstruct" calls I see. It appears we are using both. What's the difference?

@loganharbour
Copy link
Member Author

I don't see anything concerning here. The only thing I didn't understand as a whole is the "clone" versus "copyConstruct" calls I see. It appears we are using both. What's the difference?

Clone duplicates the input parameters and creates a new object from those parameters (less chance at screwing up state). Copy construct is really only used in two cases: direct copy construct for mesh and relationship managers

@loganharbour loganharbour dismissed lindsayad’s stale review March 23, 2024 20:34

App failures addressed

@loganharbour loganharbour merged commit 091245d into idaholab:next Mar 23, 2024
100 of 103 checks passed
@loganharbour loganharbour deleted the disallow_characters branch March 23, 2024 20:34
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.

Improve mooseError() to include as much input info as possible
5 participants