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

Bug Fix: Checkpoint file size #528

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Conversation

feathern
Copy link
Contributor

This fixes a bug in Rayleigh where a checkpoint file can appear to be too large if the file existed and was overwritten in a subsequent run with lower resolution. This PR ensures that the file is deleted before being written to, preventing this bug.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

The rationale makes sense to me, but I have a question about the implementation, see above.

@@ -1573,6 +1574,11 @@ Subroutine Write_Data(self,disp,file_unit,filename)
If (present(filename)) Then
! The file is not open. We must create it.
If (self%output_rank) Then
If (present(clear_existing)) Then
If (clear_existing) Then
Call MPI_File_delete(filename, MPI_INFO_NULL, ierr)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you are currently doing this unconditionally no matter if the file is present or not. Can MPI_File_delete handle the case if the file is not present? Or is it only called if the file exists and I missed the place where that is checked?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed this might be easier if you call MPI_FILE_set_size(funit, 0, ierr) after the call to MPI_FILE_OPEN, this would avoid the case where you dont know if the file already exists (because MPI_FILE_OPEN just created it).

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Looks good to go now, thanks for making the change. Feel free to merge at any time when you are comfortable with the tests on Pleiades.

@illorenzo7
Copy link
Contributor

OK, this looks good and fixes the bug I was encountering.

@illorenzo7 illorenzo7 merged commit 0de7299 into geodynamics:main Jun 18, 2024
7 checks passed
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.

3 participants