-
Notifications
You must be signed in to change notification settings - Fork 97
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
Replace format labels with strings #253
Conversation
Codecov Report
@@ Coverage Diff @@
## main #253 +/- ##
=======================================
Coverage 40.77% 40.77%
=======================================
Files 13 13
Lines 3882 3882
=======================================
Hits 1583 1583
Misses 2299 2299 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Left a few comments, but I am stopping now to give my big picture feedback: I think we can improve the consistency here a bit. I like the module idea, but I am not sure how necessary it is. Primarily because it looks like we don't need to move all formats to the module, but then again, how do we decide some lives in there compared to the files themselves?
I was going to recommend renaming the format module but I think my current recommendation would be to remove the format module and if a format is used more than one time in a file, define it as a variable. If it is used once, just typing it in the write/print statements is fine.
Let me know what you think. Some of my individual comments are related to this.
Finally, just so that we document this here: Tapenade produced non F2008 standard code when the failure check in BCData was ignored in a certain way. I pushed a fix for this in this PR: fbe2224#diff-9c42070cc0184316dd2c54cc31a4b7fdf4d3b89ee7493c148b844c3b42831d86
In the future, as @sseraj recommended to me in private, we may want to either consider using different compiler flags for the tapenade generated code, or tweak tapenade options to generate only F2008 valid code. I am guessing this issue will be automatically addressed when we update the tapenade version.
src/adjoint/adjointUtils.F90
Outdated
|
||
! Write the residual norm to stdout every adjMonStep iterations. | ||
|
||
if(mod(n, adjMonStep) ==0 ) then | ||
if( myid==0 ) write(*, 10) n, rnorm | ||
if( myid==0 ) write(*, monitorFormat) n, 'KSP Residual norm', rnorm |
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.
I also realize this is a way to do this formatting. Why didn't you do this everywhere? E.g. in adjoint API, there are formats that repeat. Maybe lets also change those to variables?
src/adjoint/adjointAPI.F90
Outdated
else | ||
write(*,40) "PETSc solver converged after", adjConvIts, & | ||
"iterations." | ||
write(*, "(1X, A, 1X, I5, 1X, A)") "PETSc solver converged after", adjConvIts, "iterations." |
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.
It looks like there's some repeated usage in this file. Any particular reason you did not define a variable for these?
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.
I agree, we when we have repeated patterns we should make this a variable, especially for related operations.
src/modules/format.F90
Outdated
@@ -0,0 +1,27 @@ | |||
module format | |||
! | |||
! This module contains common format specifiers for printing output. |
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.
What is the rule to decide if a format will be defined in here compared to the files they are used? If you have a rule, can you please explain it so people can follow it? If not, we can move the declarations here, but that might be unnecessary effort.
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.
I would say if the same formatting patterns are spanning multiple subroutines within a module then they should be a module variable in the respective module, and if they are spanning multiple modules, it should be moved into this module. The patterns that are currently here should work for a lot of cases.
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.
Using a module was originally @eirikurj's idea, so maybe he can comment on my implementation. I put format strings that showed up across different files in the format module, but there ended up being only a few common ones.
I also agree that I was not very consistent in using variables and directly using strings. I would be fine with doing what you suggested with the variables.
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.
I think it looks good overall, but as @anilyil mentioned, there are perhaps places that could be improved.
On the module discussion, I think we should utilize a module for these very common patterns, instead of having the same pattern or similar patters across files. This makes it easier to change and maintain, e.g., by changing print precision etc.
However, I do think that there is a tradeoff, and we should not necessarily force everything to be a subroutine or a module variable, so just defining the pattern for these one-off print/write cases should also be fine.
src/modules/format.F90
Outdated
@@ -0,0 +1,27 @@ | |||
module format | |||
! | |||
! This module contains common format specifiers for printing output. |
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.
I would say if the same formatting patterns are spanning multiple subroutines within a module then they should be a module variable in the respective module, and if they are spanning multiple modules, it should be moved into this module. The patterns that are currently here should work for a lot of cases.
src/adjoint/adjointAPI.F90
Outdated
else | ||
write(*,40) "PETSc solver converged after", adjConvIts, & | ||
"iterations." | ||
write(*, "(1X, A, 1X, I5, 1X, A)") "PETSc solver converged after", adjConvIts, "iterations." |
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.
I agree, we when we have repeated patterns we should make this a variable, especially for related operations.
Thanks for the feedback. I will make changes to specify the format patterns more consistently using the following convention:
|
I think I addressed all the comments. One exception is that I kept instances of I also checked the outputs again after the changes. |
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.
Did another quick pass. Left a few very minor comments.
src/adjoint/adjointAPI.F90
Outdated
@@ -1,5 +1,10 @@ | |||
module adjointAPI | |||
|
|||
use constants |
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.
Since its imported here at the top, we could remove the same statement from the subroutines.
Also, if its not too difficult, it would be great to use only
, although its less important for this module.
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.
Fixed
Thanks for the comments @eirikurj. I will address them by the end of the week. |
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.
Looks good!
Purpose
This PR replaces all format labels with strings. This is necessary to get idempotent formatting with fprettify (that is, the formatting will not change when fprettify is run multiple times in a row on the same code). This is a separate PR from #229 because these changes are manual whereas the fprettify changes will be automated.
If I set the format strings correctly, the output should look the same as before. I checked that this is true for common outputs, like load balancing, cell count, NK switch, adjoint monitor, and negative volumes.
Expected time until merged
One week. This is blocking #229 so it is a bit urgent.
Type of change
Testing
Test coverage for output is non-existent, so it would be great if reviewers could run this code on their regular cases and check that the output looks the same.
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable