-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add time-stacked output streams in IOStreams #146
base: develop
Are you sure you want to change the base?
Add time-stacked output streams in IOStreams #146
Conversation
This update is a subsequent PR for IOStreams. - This update introduces time-stacked output streams in IOStreams by incorporating the 'FileFreq' and 'FileFreqUnits' options in the 'omega.yml' file. - Additionally, this update enables multiple IO 'FreqUnits' separated by a delimiter ',' (e.g., 'FreqUnits = months,OnStartup').
@hyungyukang, I assume you've noticed that the code needs some linting before clang-format is happy. |
@hyungyukang, did you also need to add a |
Found a minor bug. I will fix it soon and run ctest again. |
Thanks. I fixed it! |
At this moment, writing output is only done by ctest. But I've been testing this PR with the manufactured solution in another branch that is built on top of this PR. I will share it with you soon, and you can use it with Polaris to test the time-stacked output streams functionality of this PR. |
Thanks @hyungyukang. I look forward to the testing branch for manufactured solution when you can point me to it. |
Fixed a bug and passed ctest on Frontier CPU & GPU. |
@xylar , I have updated the branch for the manufactured solution that is built on top of this PR: https://github.com/hyungyukang/Omega/tree/hkang/omega/iostream-manufactured However, I was not able to test the branch with Polaris as Perlmutter is under maintenance today. I will test it later when Perlmutter is back in service, but I'll be on a plane tomorrow. Meanwhile, please test on your end and let me know if you have any problems. |
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.
@hyungyukang Thanks for this and I can confirm that the unit tests work. I recognize this was meant to be a quick fix and I'm willing to approve it as such for the short term if this is a priority. It does require some changes to the documentation to document the new options.
However, I suspect this isn't a robust solution and there are some changes I would want to see in a permanent solution, whether we want to take the time now or refactor in a later PR. For example, I'm not sure you'll get the behavior you want if you restart in middle of a file interval? And it would be safer to not keep a file open for the duration. I'm happy to work on a more permanent solution with some required changes to the base IO layer and Dims for better support of file "Append" options and unlimited dimensions/frames. But if it's important to have this working in the near term, I think it's probably ok as long as the restrictions are documented.
@philipwjones and @hyungyukang, for my part, I'd rather have a robust solution than a quick one. Let's see what @vanroekel thinks. |
I agree with @xylar, looking at this, I don't think this is a feature we want to do quick. From my understanding I don't think we have tests with a pressing need for multiple times in the I/O. So let's wait on this until we can get Phil's concerns fully addressed. |
@philipwjones , I agree. I also prefer the 'Append' approach for more robust support of time-stacked output streams. While I can address your concerns, making changes to the base IO and Dims within this PR would be challenging. As @vanroekel mentioned, the time-stacked output stream feature is not an urgent task, so I am inclined to close this PR for now, and you can open another PR later. In the meantime, I believe the split output files can also be read in Polaris, correct @xylar ? |
Yes, that should not be an issue. |
This update is a subsequent PR for IOStreams.
FileFreq
andFileFreqUnits
options in theomega.yml
file.FreqUnits
to be used withOnStartup
option, separated by a comma ',' (e.g.,FreqUnits = months,OnStartup
).Unit tests passed on Frontier CPU and GPU after applying @grnydawn 's hot fix for Frontier's core module updates as of Oct 15: https://acmeclimate.slack.com/archives/C010KQD6S5T/p1728920431549039 (download
config_machines.xml
and replace it withOmega/cime_config/machines/config_machines.xml
)Checklist