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

Add more context to errors - mesh/ files #2450

Merged
merged 119 commits into from
Nov 2, 2023

Conversation

MelReyCG
Copy link
Contributor

@MelReyCG MelReyCG commented May 16, 2023

This PR uses the features added the PR #2404 to add more context to the errors messages of classes in src/coreComponents/mesh 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

MelReyCG and others added 27 commits March 23, 2023 14:17
- 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
+ virtual destructors
Copy link
Contributor

@TotoGaz TotoGaz left a comment

Choose a reason for hiding this comment

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

I'll move this out of the merge queue because there are some pending questions.

@@ -24,7 +24,7 @@ CellElementRegion::CellElementRegion( string const & name, Group * const parent
ElementRegionBase( name, parent )
{
registerWrapper( viewKeyStruct::sourceCellBlockNamesString(), &m_cellBlockNames ).
setInputFlag( InputFlags::OPTIONAL );
setInputFlag( InputFlags::REQUIRED );
Copy link
Contributor

Choose a reason for hiding this comment

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

This will change the rst file. And maybe will require a rebaseline.

@MelReyCG
Copy link
Contributor Author

MelReyCG commented Sep 21, 2023

What really matters is

code_width = 200 # unsigned number

@TotoGaz Just to clarify, are you talking about string values or about every code lines? Is their an exception for string values?
Do you want me to remove unnecessary line returns ?

@rrsettgast
Copy link
Member

What really matters is

code_width = 200 # unsigned number

@TotoGaz Just to clarify, are you talking about string values or about every code lines? Is their an exception for string values? Do you want me to remove unnecessary line returns ?

There is no hard limit. We were trying to stay under 100 characters, but that even seems low now. I was purposefully lenient. 120 is probably more realistic as a goal. Of course sometimes it makes sense to go longer, so it is allowed. It is at your discretion.

@paveltomin
Copy link
Contributor

@MelReyCG clean up and move back into merge queue?

@rrsettgast rrsettgast added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Oct 26, 2023
@rrsettgast
Copy link
Member

@TotoGaz is this good to go in?

@MelReyCG
Copy link
Contributor Author

MelReyCG commented Oct 27, 2023

@sframba ran the integrated tests on it (thanks!), and 5 tests failed:

  • acoustic_vti_smoke_0{1,8}
  • particleRepartition
  • dfgMovingGrid
  • singleParticle

The first two are potentially failing due to another PR, and we don't think that the three last failed because of this PR.

@CusiniM
Copy link
Collaborator

CusiniM commented Oct 27, 2023

@sframba ran the integrated tests on it (thanks!), and 5 tests failed:

* acoustic_vti_smoke_0{1,8}

* particleRepartition

* dfgMovingGrid

* singleParticle

The first two are potentially failing due to another PR, and we don't think that the three last failed because of this PR.

this should be fixed in the latest develop

@paveltomin paveltomin requested a review from TotoGaz November 2, 2023 21:19
Copy link
Contributor

@TotoGaz TotoGaz left a comment

Choose a reason for hiding this comment

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

I'm fine merging this, there are questions that can be answered afterward.

@@ -109,6 +109,9 @@ ENUM_STRINGS( ElementType,
"HendecagonalPrism",
"Polyhedron" );

/// String available for mesh errors
inline auto constexpr generalMeshErrorAdvice = "\nPlease consider checking the validity of your mesh with the `mesh_doctor` GEOS python tools.";
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, could you add the URL https://geosx-geosx.readthedocs-hosted.com/en/latest/docs/sphinx/pythonTools/mesh_doctor.html?
Also, could you remove the initial \n?

@@ -140,7 +140,7 @@ void InternalMeshGenerator::postProcessInput()
}
if( failFlag )
{
GEOS_ERROR( "vertex/element mismatch InternalMeshGenerator::ReadXMLPost()" );
GEOS_ERROR( getDataContext() << ": vertex/element mismatch." << generalMeshErrorAdvice );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GEOS_ERROR( getDataContext() << ": vertex/element mismatch." << generalMeshErrorAdvice );
GEOS_ERROR( getDataContext() << ": vertex/element mismatch.\n" << generalMeshErrorAdvice );

Then this would be it?

@@ -531,7 +534,7 @@ loadMesh( Path const & filePath,
case VTKMeshExtension::pvti: return parallelRead( vtkSmartPointer< vtkXMLPImageDataReader >::New() );
default:
{
GEOS_ERROR( extension << " is not a recognized extension for VTKMesh. Please use .vtk, .vtu, .vtr, .vts, .vti, .pvtu, .pvtr, .pvts or .ptvi." );
GEOS_ERROR( extension << " is not a recognized extension for VTKMesh. Please use ." << EnumStrings< VTKMeshExtension >::concat( ", ." ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous list did not contain vtm, the new list does. Is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as it is managed by this switch, I think we should list vtm as a compatible extension.

"Make sure partitionRefinement is set to 1 or higher (this may help).",
pointGlobalID ) );
GEOS_FMT( "At least one duplicate point detected (globalID = {}).{}\n"
"Potential fixes :\n- Consider cleaning the dataset in Paraview using 'Clean to grid' filter.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to advise the usag of clean to grid? My XP with this tool was not great. @jeannepellerin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked her, we don't need to recommend that to the user as it is not reliable. I will remove this advice.

@TotoGaz TotoGaz merged commit c7add46 into develop Nov 2, 2023
18 checks passed
@TotoGaz TotoGaz deleted the feature/rey/error-xml-line-mesh branch November 2, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs flag: ready for review
Projects
None yet
6 participants