Fixed CGNS deallocation

sseraj commented 6 months ago

Purpose

The new Intel compilers we are testing in mdolab/docker#266 revealed bugs in the CGNS API deallocation routine. I fixed the bugs by adding more deallocation safeguards and assigning the correct value for nZones. I also removed the nullify calls for some attributes because they are not needed and only work with pointer types, not allocatable.

Expected time until merged

2-3 days

Type of change

[x] 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

I pulled the u22-intel-impi-latest-amd64-failed image from the Docker PR and tested that the tests pass with this fix.

Checklist

[ ] I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
[x] I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
[x] 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

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 50.11%. Comparing base (4a8a14e) to head (faf0241). Report is 2 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #34 +/- ## ======================================= Coverage 50.11% 50.11% ======================================= Files 5 5 Lines 1688 1688 ======================================= Hits 846 846 Misses 842 842 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sseraj commented 6 months ago

It's not a bug in the CGNS library but in how pySurf reads and stores the CGNS mesh, so no need to open a bug report

eirikurj commented 5 months ago

@sseraj overall looks good. However, there are other instances of pointers floating around, but I doubt that there is a need for that. Am I missing something?

sseraj commented 5 months ago

Do you mean instances where there is a pointer that should be an allocatable instead? This is true for the bctype type, but I think the BC implementation is incomplete: https://github.com/mdolab/pysurf/blob/9c7de8dd9be67db4890e3c6f3825a61201851b33/src/CGNSInterface/cgnsInterface.F90#L636

Should I convert the pointers to allocatables (and deallocate them) for good measure?

sseraj commented 5 months ago

@eirikurj Could you clarify your comment?

eirikurj commented 5 months ago

Should I convert the pointers to allocatables (and deallocate them) for good measure?

Yes, I think if we dont need pointer specific features (which I dont think is the case here) changing them to allocate would improve the code. Is this only relevant to the incomplete implementation? Not sure its worth changing something that is commented out and we have no way of testing. Can we uncomment and implement this? Not sure how much effort is needed here or if this is needed. If not, I would prefer that any unused related variables are also commented out.

sseraj commented 5 months ago

@eirikurj I changed the remaining pointers in CGNSInterface to allocatables and commented out the unused variables

mdolab / pysurf