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

Allow I/O of multi-tile arrays and fields with undistributed dimensions #110

Merged
merged 4 commits into from
Dec 16, 2022

Conversation

billsacks
Copy link
Member

This PR extends the IO routine undist_arraycreate_alldist to work with multi-tile arrays. This allows I/O of multi-tile arrays and fields with undistributed dimensions.

The implementation follows suggestions from @theurich and @oehmke .

I have tested this using billsacks/esmfprojects-multi_tile_io@eea516b. This writes a 6-tile field with 2 ungridded dimensions (dimensions 1 & 3 are gridded, 2 & 4 ungridded). I have manually inspected the output to verify that this field is being written correctly.

@billsacks billsacks requested a review from theurich December 10, 2022 13:33
@billsacks billsacks self-assigned this Dec 10, 2022
@billsacks
Copy link
Member Author

I am marking this as a draft PR. Things still needed:

  • I will add a unit test of this functionality
  • I'd like @uturuncoglu to test this and make sure this works for him

@theurich
Copy link
Member

@billsacks, this looks exactly as expected! While going through the code I made a tiny code simplification that I just pushed. Nothing critical, but since we are in there anyway, might as well.
One thing I was going back and forth was the question whether both single- and multi-tile cases should call into the same DistGrid::create(), which we could do. It would eliminate having to do the conditional branching. On the other hand, there are a few extra things to do for multi-tile, also down in the DistGrid::create() itself. So it might just be smarter to do the branching on this level as you have it. That is where I came out thinking about it now, so I think what you have is the right way to go.

@billsacks billsacks force-pushed the multitile_undist_redist branch from 915d0bf to f4deda9 Compare December 13, 2022 00:03
@billsacks
Copy link
Member Author

Thanks for the change @theurich . I had some local changes to update to latest develop (in an attempt to try to get things working with the issues in #112 ), so I ended up cherry-picking your changes onto this branch and then force pushing. (Let me know if this messes you up in any way, in which case I can undo this and redo it differently.)

@theurich
Copy link
Member

@billsacks - good with me.

@billsacks
Copy link
Member Author

One thing I was going back and forth was the question whether both single- and multi-tile cases should call into the same DistGrid::create(), which we could do. It would eliminate having to do the conditional branching. On the other hand, there are a few extra things to do for multi-tile, also down in the DistGrid::create() itself. So it might just be smarter to do the branching on this level as you have it. That is where I came out thinking about it now, so I think what you have is the right way to go.

Thanks for your thoughts on this point. I wondered about this when I was doing the implementation. I hadn't been totally sure if it would work to use the same call for the single-tile case, but even given that it would, I saw some advantage to keeping the standard single-tile case simple and easier to understand... so, basically, the same feelings you have.

@billsacks billsacks marked this pull request as ready for review December 15, 2022 22:49
@billsacks
Copy link
Member Author

I have added unit tests of the new code, and it sounds like this is working for @uturuncoglu . I'm still working on the issue that @uturuncoglu identified – that we get warning messages like

20221213 104527.508 WARNING          PET144 ESMCI_PIO_Handler.C:1567 ESMCI::PIO_Handler::isOpen()   File, ufs.cpld.lnd.ini.2000-01-01-00000.tile1.nc, closed by PIO

However, I have found that this warning exists on develop, too. I'm looking into it to see if it's something caused by my earlier multi-tile I/O work, but I feel like the fix for that can (and maybe should) be separate from this PR.

So @theurich please let me know if this is okay to merge from your perspective.

Copy link
Member

@theurich theurich left a comment

Choose a reason for hiding this comment

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

@billsacks - please go ahead bringing this into develop.

@uturuncoglu
Copy link
Member

uturuncoglu commented Dec 15, 2022 via email

@theurich
Copy link
Member

I agree about the snapshot. My regular routine with respect to snapshots now is that I look on Saturday how the nightlies did (Fri->Sat). When looking good, I put a new tag onto develop. By Monday then we typically have a pretty complete test column for that new tag.
Assuming @billsacks merged this PR today, we'll have a snapshot tag with it by Monday following the outlined procedure.

@billsacks billsacks merged commit 3b11fab into develop Dec 16, 2022
@billsacks billsacks deleted the multitile_undist_redist branch December 16, 2022 00:39
@theurich
Copy link
Member

@uturuncoglu - just to let you know that v8.5.0b10 includes the multi-tile PR merge. There were some hicups in the summarizer of the nightlies over the weekend, but I addressed that issue this morning. The summary table suggests that v8.5.0b10 is good to be used.

@uturuncoglu
Copy link
Member

@theurich Thank. That is great. I'll open an issue in UFS side to have v8.5.0b10 at least on Orion and Cheyenne.

@uturuncoglu
Copy link
Member

@theurich You can find the issue in the following link as for your reference,

ufs-community/ufs-weather-model#1542

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