Code tidying #907
Code tidying #907
Conversation
PR Reviewer Guide 🔍(Review updated until commit caa51db)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #907 +/- ##
==========================================
+ Coverage 44.02% 44.03% +0.01%
==========================================
Files 69 69
Lines 19625 19644 +19
Branches 2430 2436 +6
==========================================
+ Hits 8640 8651 +11
- Misses 9484 9488 +4
- Partials 1501 1505 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Failed on AMD GPU hardware with Cray compilers because: |
|
I will review this soon. It's hard to review because I want to be sure the tests caught all the ways this could have possibly broken something. |
| call nvtxRangePush(tempName) | ||
| else | ||
| event%color = col(mod(id, 7) + 1) | ||
| if (present(id)) then |
There was a problem hiding this comment.
does this work, too? you need to run nsys with cpu and gpu modes to find out
There was a problem hiding this comment.
This is one output with --nsys and --gpu
Now that each row represent a specific NVTX range, I think this code works properly.
[3/8] Executing 'nvtx_sum' stats report
Time (%) Total Time (ns) Instances Avg (ns) Med (ns) Min (ns) Max (ns) StdDev (ns) Style Range
-------- --------------- --------- --------------- --------------- ------------- ------------- ----------- ------- ---------------------
22.5 2,208,358,145 1 2,208,358,145.0 2,208,358,145.0 2,208,358,145 2,208,358,145 0.0 PushPop SIMULATION-TIME-MARCH
19.1 1,867,369,618 1 1,867,369,618.0 1,867,369,618.0 1,867,369,618 1,867,369,618 0.0 PushPop INIT
13.0 1,274,854,743 1 1,274,854,743.0 1,274,854,743.0 1,274,854,743 1,274,854,743 0.0 PushPop INIT-MODULES
12.5 1,223,361,067 50 24,467,221.3 23,591,994.0 21,520,831 50,213,333 3,972,174.3 PushPop TIMESTEP
8.3 817,515,022 1 817,515,022.0 817,515,022.0 817,515,022 817,515,022 0.0 PushPop SAVE-DATA
6.5 637,690,738 150 4,251,271.6 4,070,501.5 3,991,106 27,681,810 1,927,606.5 PushPop COMPUTE-RHS
6.0 591,356,493 1 591,356,493.0 591,356,493.0 591,356,493 591,356,493 0.0 PushPop INIT-MPI
4.3 421,111,172 1 421,111,172.0 421,111,172.0 421,111,172 421,111,172 0.0 PushPop MPI:MPI_Init
3.6 349,779,183 450 777,287.1 764,172.0 749,605 3,553,709 132,730.3 PushPop RHS-ADVECTION-SRC
1.5 142,218,607 450 316,041.3 303,346.0 282,750 4,176,763 182,804.3 PushPop RHS-WENO
0.8 81,742,288 450 181,649.5 165,865.5 160,829 6,887,448 316,954.3 PushPop RHS-RIEMANN-SOLVER
0.7 66,194,030 1 66,194,030.0 66,194,030.0 66,194,030 66,194,030 0.0 PushPop FINALIZE-MODULES
0.6 62,960,296 1 62,960,296.0 62,960,296.0 62,960,296 62,960,296 0.0 PushPop MPI:MPI_Finalize
0.3 30,719,602 150 204,797.3 164,348.0 160,500 6,002,360 476,562.6 PushPop RHS-COMMUNICATION
0.1 11,811,929 150 78,746.2 76,288.5 74,967 317,997 19,778.8 PushPop RHS-ACOUSTIC-SRC
0.1 10,396,748 150 69,311.7 44,600.0 41,059 3,761,697 303,513.2 PushPop RHS-CONVERT
0.0 1,134,973 1 1,134,973.0 1,134,973.0 1,134,973 1,134,973 0.0 PushPop INIT-GPU-VARS
0.0 225,778 745 303.1 322.0 163 1,897 119.6 PushPop MPI:MPI_Bcast
0.0 144,716 450 321.6 311.5 172 748 84.4 PushPop RHS-HYPOELASTICITY
0.0 125,007 450 277.8 265.0 152 602 76.3 PushPop RHS-MHD
0.0 75,393 51 1,478.3 838.0 721 7,647 1,806.1 PushPop MPI:MPI_Barrier
0.0 72,677 200 363.4 326.0 164 5,742 391.1 PushPop RHS-ELASTIC
| @@ -1261,9 +1261,6 @@ contains | |||
| !! Determines the amount of freedom available from utilizing a large | |||
| !! value for the local curvature. The default value for beta is 4/3. | |||
|
|
|||
| real(wp), parameter :: alpha_mp = 2._wp | |||
There was a problem hiding this comment.
do these not get used anywhere?
There was a problem hiding this comment.
In the subroutine, defined both alpha and alpha_mp with the same value, but only used the alpha_mp. They are literally the same, so I just removed the redundant one.
|
comments are unresolved and now there are merge conflicts |
|
merge conflicts (probably due to my refactoring to get rid of masking parent variables) |
|
please review the comments i left above 3 weeks ago. e.g., |
There was a problem hiding this comment.
Pull Request Overview
This PR performs code tidying to fix compiler warnings in the MFC Fortran codebase, primarily by removing unused variables, correcting MPI data type declarations, and adding conditional compilation guards. The changes address various issues raised during debug builds while maintaining code functionality.
- Remove unused variables, parameters, and dummy arguments throughout the codebase
- Fix MPI data type declarations to use proper address kinds instead of regular integers
- Add conditional compilation guards to prevent unused variable warnings
- Correct parameter order in finite difference coefficient function calls
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| m_weno.fpp | Remove duplicate parameter declarations and use existing alpha/beta variables |
| m_time_steppers.fpp | Comment out unused gradient magnitude variables |
| m_start_up.fpp | Fix integer conversion and conditional variable declaration |
| m_riemann_solvers.fpp | Remove unused xi_field variables from private clause |
| m_rhs.fpp | Comment out unused gradient magnitude allocations |
| m_mpi_proxy.fpp | Remove unused loop variables and communication flags |
| m_mhd.fpp | Remove unused derivative arrays and fix function parameter order |
| m_hypoelastic.fpp | Fix parameter order in finite difference function calls |
| m_hyperelastic.fpp | Fix parameter order in finite difference function calls |
| m_fftw.fpp | Add conditional compilation for OpenACC-specific variables |
| m_derived_variables.fpp | Fix parameter order and GPU update device references |
| m_data_output.fpp | Remove unused stability criterion variables and add directory handling |
| m_compute_cbc.fpp | Remove unused loop variable |
| m_patches.fpp | Remove unused parameters from airfoil subroutines |
| m_compute_levelset.fpp | Remove unused length_z variable |
| ExtrusionHardcodedIC.fpp | Remove unused variables and improve floating-point comparisons |
| m_start_up.f90 | Fix parameter order in finite difference function calls |
| m_data_input.f90 | Remove unused MPI offset and loop variables |
| m_variables_conversion.fpp | Add conditional compilation guards and improve variable initialization |
| m_phase_change.fpp | Remove unused parameter from relaxation solver |
| m_nvtx.f90 | Fix variable scope and improve conditional compilation |
| m_mpi_common.fpp | Initialize variables and add conditional guards for different compilation modes |
| m_helper_basic.fpp | Improve precision-specific tolerance handling |
| m_finite_differences.fpp | Fix parameter order and add optional parameter handling |
| m_checker_common.fpp | Add conditional compilation guard for unused variable |
| m_boundary_common.fpp | Fix MPI data types and remove unused variables |
| $:GPU_UPDATE(device='[fd_coeff_x]') | ||
|
|
||
| fd_order, fd_number) | ||
| $:GPU_UPDATE(device='[fd_coeff_z]') |
There was a problem hiding this comment.
This should be updating fd_coeff_x, not fd_coeff_z. The variable being allocated is fd_coeff_x but the GPU update references fd_coeff_z.
| $:GPU_UPDATE(device='[fd_coeff_z]') | |
| $:GPU_UPDATE(device='[fd_coeff_x]') |
Copilot uses AI. Check for mistakes.
| fd_number, fd_order) | ||
| $:GPU_UPDATE(device='[fd_coeff_y]') | ||
| fd_order, fd_number) | ||
| $:GPU_UPDATE(device='[fd_coeff_z]') |
There was a problem hiding this comment.
This should be updating fd_coeff_y, not fd_coeff_z. The variable being allocated is fd_coeff_y but the GPU update references fd_coeff_z.
| $:GPU_UPDATE(device='[fd_coeff_z]') | |
| $:GPU_UPDATE(device='[fd_coeff_y]') |
Copilot uses AI. Check for mistakes.
| do i = 1, 2 | ||
| Re_K(i) = dflt_real | ||
| end do | ||
| #ifdef MFC_SIMULATION |
There was a problem hiding this comment.
This loop initializes Re_K(i) but Re_K is not declared in this scope without MFC_SIMULATION. The Re_K initialization should be moved inside the MFC_SIMULATION conditional block.
| do i = 1, 2 | |
| Re_K(i) = dflt_real | |
| end do | |
| #ifdef MFC_SIMULATION | |
| #ifdef MFC_SIMULATION | |
| do i = 1, 2 | |
| Re_K(i) = dflt_real | |
| end do |
Copilot uses AI. Check for mistakes.
| @@ -132,9 +134,17 @@ contains | |||
| real(wp) :: e_Per_Kg, Pdyn_Per_Kg | |||
| real(wp) :: T_guess | |||
| real(wp), dimension(1:num_species) :: Y_rs | |||
| #:if not chemistry | |||
There was a problem hiding this comment.
Use standard Fortran preprocessing directives. The Fypp syntax '#:if not chemistry' should follow the project's established preprocessing patterns. Based on other conditional blocks in the file, this should likely use #ifdef/#ifndef directives.
Copilot uses AI. Check for mistakes.
PR Code Suggestions ✨No code suggestions found for the PR. |
Oh I forgot to submit the review changes. Those comments can be published now. |
|
@wilfonba, can you look at this PR for anything suspicious? I have two (possibly unlikely) worries:
It's not so hard to look through the changes, and most seem benign, but some are higher risk and could truly break the code for a specific kind of simulation. |
I'll add it to my list for tomorrow or this weekend. |
|
Nothing sticks out as obviously problematic to me. Apologies for the delay in response |
User description
Description
This is the latest progress of code tidying.
Fixes #845
Type of change
./mfc.sh build --debugA few issues remaining
For example:
Adding a local variable only to silence a warning:
which I feel is not necessary.
@:analytical()macro during./mfc.sh test.An example is attached:
PR Type
Bug fix, Enhancement
Description
Fixed compiler warnings and unused variable declarations
Corrected MPI data type mismatches and function signatures
Improved precision handling for floating point comparisons
Removed redundant variable allocations and declarations
Diagram Walkthrough
File Walkthrough