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

IO Bug Fix #510

Merged
merged 1 commit into from
Jun 12, 2024
Merged

IO Bug Fix #510

merged 1 commit into from
Jun 12, 2024

Conversation

feathern
Copy link
Contributor

This fixes a subtle bug in Rayleigh. When opening a file for parallel I/O, Rayleigh records the error code. It was intended that if a nonzero error code was found, so that the file_open variable remained false, no subsequent file operations would be performed. A check on the file_open variable was missing, however, for everything but the operations related to writing the file header data. This PR fixes this bug.

I've been emailing with @illorenzo7 and the NASA HECC staff about this already today, so I'm assigning the review to him.

-Nick

p.s. I also removed a commented line of debugging code.

Copy link
Contributor

@illorenzo7 illorenzo7 left a comment

Choose a reason for hiding this comment

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

@feathern This looks good, and I am starting to understand some of the issues my job experienced today (i.e., unable to open a file from disk but Rayleigh still was trying to write to that file). One question: in our conversation with Johnny you said the error was "due to an 'if' statement that didn't include all of the code meant to be contained in the conditional." But here you made a new if block. Is there a reason 781-783 can't be included in the prior if-block (which also has the "responsible" variable)?

@feathern
Copy link
Contributor Author

It's not actually the same conditional. One is (responsible and file_open), while the other is just (file_open). The "responsible" node writes the header information (timestep, time, record number) for the record, so we don't want all processors doing it. Also, I'm really just trying to patch something problematic quickly right now -- we need to work on various aspects of this at the hackathon (such as the 'wait' feature we discussed).
-Nick

Copy link
Contributor

@illorenzo7 illorenzo7 left a comment

Choose a reason for hiding this comment

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

Sounds good! Mainly I was just curious, I figured the separate blocks were intentional. Merging.

@illorenzo7 illorenzo7 merged commit ce66ed0 into geodynamics:main Jun 12, 2024
2 of 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.

2 participants