-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add more context to errors - physicsSolvers/ files #2448
Conversation
- Added SourceContext to know where Groups/Wrappers has been declared - Added a test for the node & attribute line infos
…y/2320-xml-errors-feedback" - using getSourceContext() instead of getPath()
…ack-3' into feature/rey/error-xml-line-2-errors-impacting
- also renaming FileContext to DataFileContext
- code refactoring + doc edit
+ virtual destructors
…l-line-physicsSolvers
(used string_view to support raw string loading)
…S-DEV/GEOS into feature/rey/error-xml-line-2
…xml-line-physicsSolvers
…rey/error-xml-line-physicsSolvers
…xml-line-physicsSolvers
…xml-line-physicsSolvers
<< "the min elevation is " << globalMinElevation[equilIndex] << " and the max elevation is " << globalMaxElevation[equilIndex] << "\n" | ||
<< "But, a datum elevation of " << datumElevation << " was specified in the input file to equilibrate the model.\n " | ||
<< "The simulation is going to proceed with this out-of-bound datum elevation, but the initial condition may be inaccurate." ); | ||
SinglePhaseBase::catalogName() << " " << getDataContext() << |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for being late with this suggestion
"..Base" classes often do not meaningful catalogName defined leading to some confusing info printed, example from other place:
SolverBase compflow (co2_flux_3d.xml, l.5): currently, GEOS assumes that there is only one mobile phase when computing the hydrostatic pressure
SolverBase here is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the Solver
term be clearer or should I totally remove the class name from here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is more general than this specific place: you probably should not use catalogName from "...Base" classes because it is not well defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be satisfying to use the same mecanism we have in ConstitutiveBase
at the SolverBase
level ? ( the virtual string getCatalogName()
method that gives the instanciated catalogName
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, for me printing solver name (getName, seems already included in getDataContext) is good enough to be able to locate the issue in the input file.
What you proposing is also fine, as long as it prints final catalog name and not some intermediate misleading info.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d184d82#r130545002 example when getName is simple and straightforward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is here: #2783 feel free to review :)
This PR uses the features added the PR #2404 to add more context to the errors messages of classes in
src/coreComponents/physicsSolvers
files.In particular, this PR aims to add the lines of the XML elements from which an error message comes from.
See issue and attached files describing error message changes: #2320