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

Mesh check error printout #81

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

hajdik
Copy link
Contributor

@hajdik hajdik commented Jan 23, 2024

Purpose

This changes the logic for error printouts for boundary condition checks and topology errors so all errors present in the mesh will print out. The previous behavior was to exit on the first issue, making it hard to fix multiple errors at once.

I also changed the printout that shows up at the start of a case. Originally it printed the case name as the name of the input file, but when running pyHyp with patch input there's no input file. I changed this to print the output file when there isn't an input file.

Expected time until merged

None

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Run this with a broken mesh to see if all expected errors are printed out

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@hajdik hajdik requested a review from a team as a code owner January 23, 2024 18:45
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 78.87%. Comparing base (71aa6c9) to head (fdbb4db).

Files Patch % Lines
pyhyp/pyHyp.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
- Coverage   79.48%   78.87%   -0.61%     
==========================================
  Files           4        4              
  Lines         424      426       +2     
==========================================
- Hits          337      336       -1     
- Misses         87       90       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

This needs some minor fixes and formatting with fprettify

src/3D/setup3d.F90 Outdated Show resolved Hide resolved
src/3D/setup3d.F90 Outdated Show resolved Hide resolved
src/3D/setup3d.F90 Show resolved Hide resolved
sseraj
sseraj previously approved these changes Feb 19, 2024
Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

From what I can tell, this looks good overall, although I dont have a case to test this. Just a few minor comments.

@@ -799,6 +807,11 @@ subroutine setup(fileName, fileType)
end do
end do patchLoop

! if there was an error in the BC setup stop
if (bcError) then
stop
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that this was like this before, but since this is a parallel code, we should probably call mpi_abort to exit more gracefully. However, since there are other locations, we could also defer this to a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several places where the error handling isn't done properly in parallel and MPI gives wild errors, I could make an issue to address that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, can you add ERROR here as well.

src/3D/setup3d.F90 Show resolved Hide resolved
src/3D/setup3d.F90 Outdated Show resolved Hide resolved
src/3D/setup3d.F90 Outdated Show resolved Hide resolved
@hajdik hajdik requested review from eirikurj and sseraj May 16, 2024 17:37
Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, couple of additional comments. BTW, do you have a mesh that we can add as tests that catch these cases?

@@ -799,6 +807,11 @@ subroutine setup(fileName, fileType)
end do
end do patchLoop

! if there was an error in the BC setup stop
if (bcError) then
stop
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, can you add ERROR here as well.

Comment on lines +220 to +221
else:
caseName = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there ever a case you dont have either inputFile or outputFile specified?

! if there was an error in the BC setup stop
if (bcError) then
print *, 'pyHyp exited due to one or more issues with mesh boundary conditions. &
& See above error printouts.'
Copy link
Contributor

Choose a reason for hiding this comment

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

The leading & with all the spaces will create a large gap in the string. For the line continuation to have consistent spacing, place the & right before See, i.e., &See.

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, couple of additional comments. BTW, do you have a mesh that we can add as tests that catch these cases? Would also be good to add printout to the PR description. Create #92 to track that we should update to mpi_abort at some point.

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.

None yet

3 participants