search

E3SM-Project / Omega

Next generation ocean model within E3SM
https://docs.e3sm.org/Omega/omega
Other
4 stars 5 forks source link

Add time-stacked output streams in IOStreams #146

Closed hyungyukang closed 1 day ago

hyungyukang commented 1 month ago

This update is a subsequent PR for IOStreams.

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 with Omega/cime_config/machines/config_machines.xml)

Checklist

xylar commented 1 month ago

@hyungyukang, I assume you've noticed that the code needs some linting before clang-format is happy.

xylar commented 1 month ago

@hyungyukang, did you also need to add a writeAll() call somewhere to get this to work? I'm just wondering what changes I need to make to the Polaris manufactured solution test to try this out (besides, obviously, to the yaml file).

hyungyukang commented 1 month ago

Found a minor bug. I will fix it soon and run ctest again.

hyungyukang commented 1 month ago

@hyungyukang, I assume you've noticed that the code needs some linting before clang-format is happy.

Thanks. I fixed it!

hyungyukang commented 1 month ago

@hyungyukang, did you also need to add a writeAll() call somewhere to get this to work? I'm just wondering what changes I need to make to the Polaris manufactured solution test to try this out (besides, obviously, to the yaml file).

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.

xylar commented 1 month ago

Thanks @hyungyukang. I look forward to the testing branch for manufactured solution when you can point me to it.

hyungyukang commented 1 month ago

Fixed a bug and passed ctest on Frontier CPU & GPU.

hyungyukang commented 1 month ago

Thanks @hyungyukang. I look forward to the testing branch for manufactured solution when you can point me to it.

@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.

xylar commented 1 month ago

@philipwjones and @hyungyukang, for my part, I'd rather have a robust solution than a quick one. Let's see what @vanroekel thinks.

vanroekel commented 1 month ago

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.

hyungyukang commented 1 month ago

@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 ?

xylar commented 1 month ago

In the meantime, I believe the split output files can also be read in Polaris, correct @xylar ?

Yes, that should not be an issue.

philipwjones commented 1 day ago

Closing since this PR will be superseded by a new PR for time slices now that other capabilities are in place.