Iceopen

[daveh150]

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

• [x ] Short (1 sentence) summary of your PR:
  Update ice_open to use 8 byte integers for large global grids.

• [x ] Developer(s):
  David Hebert, Naval Research Lab, Stennis Space Center, MS

• [x ] Suggest PR reviewers from list in the column to the right.
  none shown

• [x ] Please copy the PR test results link or provide a summary of testing completed below.
  results.log

• How much do the PR code changes differ from the unmodified code?

  • [x ] bit for bit
  • different at roundoff level
  • more substantial

• Does this PR create or have dependencies on Icepack or any other models?

  • Yes
  • [x ] No

• Does this PR add any new test cases?

  • Yes
  • [ x] No

• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)

  • Yes
  • [x ] No, does the documentation need to be updated at a later time?
    • Yes
    • [x ] No

• Please provide any additional information or relevant details below:

[apcraig]

This raises a number of questions in my mind. First, looking up the recl argument, it seems it's integer4 and cannot be larger than 2^31-1. That means at some point, we might not be able to continue to do this for very large grids. But I think we still have some room. But the only problem really is in the RecSize computation. Algn, RecSize, Remnant are fine as integer4 in general in this implementation. And it's probably better if they remain that way.

I also worry that some of the compilers are going to complain with algn being promted to integer8 in the interface. I would be a little more careful about that and keep everything integer4 until promotion is needed.

I suggest leaving everything as integer*4 as it was. Then just adding

integer (kind=int8_kind) :: RecSizeI8
integer (kind=int_kind),parameter :: bits_per_byte = 8 ! this can be module data or even added to ice_constants

then later

RecSizeI8 = int(nx,int8_kind) * int(ny,int8_kind) * int(nbits,int8_kind) / int(bits_per_byte,int8_kind)
RecSize = int(RecSizeI8, int_kind)

and then everything stays integer*4. I know the RecSizeI8 statement is a little messy, but it keeps the rest of the code very clean. As you suggested earlier, this can also be fixed by changing the order of operations, and that could work too although increases the chances of changing the results.

I am still open a bit about the best way to implement this. Let me know what you think.

[daveh150]

Tony, I think your solution is good, given how CICE6 opens and reads files internally. The only other way I can think to get around this is to loop over the ‘y’ and read into the array, but that is not compatible with how open and read are used throughout the code. “Stream” access may work, but then we need to worry about byte swapping. I’ll implement what you have outlined and run the tests again. That should work. Thanks, Dave David A. Hebert Ocean Sciences Division, Code 7322 Naval Research Laboratory Stennis Space Center, MS 39529 (228)688-5846; DSN 828-5846 <http://www.nrl.navy.mil/> http://www.nrl.navy.mil From: Tony Craig <notifications@github.com> Sent: Thursday, February 27, 2020 18:57 To: CICE-Consortium/CICE <CICE@noreply.github.com> Cc: David Hebert, Code 7322 <david.hebert@nrlssc.navy.mil>; Author <author@noreply.github.com> Subject: Re: [CICE-Consortium/CICE] Iceopen (#410) @apcraig requested changes on this pull request. This raises a number of questions in my mind. First, looking up the recl argument, it seems it's integer4 and cannot be larger than 2^31-1. That means at some point, we might not be able to continue to do this for very large grids. But I think we still have some room. But the only problem really is in the RecSize computation. Algn, RecSize, Remnant are fine as integer4 in general in this implementation. And it's probably better if they remain that way. I also worry that some of the compilers are going to complain with algn being promted to integer8 in the interface. I would be a little more careful about that and keep everything integer4 until promotion is needed. I suggest leaving everything as integer*4 as it was. Then just adding integer (kind=int8_kind) :: RecSizeI8 integer (kind=int_kind),parameter :: bits_per_byte = 8 ! this can be module data or even added to ice_constants then later RecSizeI8 = int(nx,int8_kind) * int(ny,int8_kind) * int(nbits,int8_kind) / int(bits_per_byte,int8_kind) RecSize = int(RecSizeI8, int_kind) and then everything stays integer*4. I know the RecSizeI8 statement is a little messy, but it keeps the rest of the code very clean. As you suggested earlier, this can also be fixed by changing the order of operations, and that could work too although increases the chances of changing the results. I am still open a bit about the best way to implement this. Let me know what you think. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#410?email_source=notifications&email_token=AE52VPE43WC5RJAXKUMGVN3RFBOOZA5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJEI4Q#pullrequestreview-366101618> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPA6KKSW3G4GZXRWJ4TRFBOOZANCNFSM4K5DLWLQ> . <https://github.com/notifications/beacon/AE52VPDJJ5QXN53RWWYQLGLRFBOOZA5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJEI4Q.gif>

[daveh150]

Actually, now that I think a bit more, if we want to avoid integer8 altogether, then I think the original method I submitted would solve that. But a question for the group is what do you prefer from a ‘clean code’ standpoint. So for reference I had (adding bits_per_byte): integer(kind=int_kind) :: nbytes integer (kind=int_kind),parameter :: bits_per_byte = 8 ! this can be module data or even added to ice_constants nbytes = nbits / bits_per_byte RecSize = nxglobal * nyglobal * nbytes Both methods will work, and to me the above is a bit cleaner from coding (with comments). But for documentation, if it is better to use what Tony suggested I’m good with that. Let me know what y’all think! Thanks, Dave David A. Hebert Ocean Sciences Division, Code 7322 Naval Research Laboratory Stennis Space Center, MS 39529 (228)688-5846; DSN 828-5846 http://www.nrl.navy.mil <http://www.nrl.navy.mil/> From: Tony Craig <notifications@github.com> Sent: Thursday, February 27, 2020 18:57 To: CICE-Consortium/CICE <CICE@noreply.github.com> Cc: David Hebert, Code 7322 <david.hebert@nrlssc.navy.mil>; Author <author@noreply.github.com> Subject: Re: [CICE-Consortium/CICE] Iceopen (#410) @apcraig requested changes on this pull request. This raises a number of questions in my mind. First, looking up the recl argument, it seems it's integer4 and cannot be larger than 2^31-1. That means at some point, we might not be able to continue to do this for very large grids. But I think we still have some room. But the only problem really is in the RecSize computation. Algn, RecSize, Remnant are fine as integer4 in general in this implementation. And it's probably better if they remain that way. I also worry that some of the compilers are going to complain with algn being promted to integer8 in the interface. I would be a little more careful about that and keep everything integer4 until promotion is needed. I suggest leaving everything as integer*4 as it was. Then just adding integer (kind=int8_kind) :: RecSizeI8 integer (kind=int_kind),parameter :: bits_per_byte = 8 ! this can be module data or even added to ice_constants then later RecSizeI8 = int(nx,int8_kind) * int(ny,int8_kind) * int(nbits,int8_kind) / int(bits_per_byte,int8_kind) RecSize = int(RecSizeI8, int_kind) and then everything stays integer*4. I know the RecSizeI8 statement is a little messy, but it keeps the rest of the code very clean. As you suggested earlier, this can also be fixed by changing the order of operations, and that could work too although increases the chances of changing the results. I am still open a bit about the best way to implement this. Let me know what you think. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#410?email_source=notifications&email_token=AE52VPE43WC5RJAXKUMGVN3RFBOOZA5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJEI4Q#pullrequestreview-366101618> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPA6KKSW3G4GZXRWJ4TRFBOOZANCNFSM4K5DLWLQ> . <https://github.com/notifications/beacon/AE52VPDJJ5QXN53RWWYQLGLRFBOOZA5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJEI4Q.gif>

[eclare108213]

I defer to the software experts to determine what the best approach is for this kind of thing. What happens if ice_open is called for a file with short integers?

[eclare108213]

spell global

[daveh150]

I just committed the method by computing nbytes first, then RecSize = nxnynbytes. To me, typecasting the 8 byte integers and then converting back to 4 byte seemed too complicated at this stage, and didn't really do anything extra since we don't use the RecSizeI8 variable after the data is computed. Please let me know if it is preferred to go back to the using RecSizeI8.

Thank you!
David

[apcraig]

I'm fine with this @daveh150. But how about making the bits_per_byte module data for now. We also need to retrigger the PR because readthedocs gagged, so a quick push should do that. Have you run any tests with this update? Thanks!

[daveh150]

Attached is the results.log with the 'nbytes' method. So far all passed, I'm waiting for 2 in the queue.

I moved the bits_per_byte to a module level variables. Those tests are underway now. I will send log when complete.
results.log

[apcraig]

This looks good. I think we can merge it when testing is done. I would like to merge this today after the JRA55 PR. My only final comment is that if nbits/bits_per_byte does not divide evenly, the new computation could produce a different result. That is partly why I was suggesting integer*8, but I don't think we'll run into that issue.

Keep us posted on test results. I hope to include this in the weekend testing.

[phil-blain]

Sorry @apcraig I won't have time to review this one.

[daveh150]

Interesting, hadn’t thought if nbits is not divisible by 8. Are we aware if anyone is using a nbits that is not divisible by 8? I guess the same problem would occur in the original method, since it was divided by 8? The integer math would not result in an accurate record length? Thanks again. Jobs are in the queue now. Gordon queue is slow at the moment. Hopefully by the weekend I’ll have results. Dave David A. Hebert Ocean Sciences Division, Code 7322 Naval Research Laboratory Stennis Space Center, MS 39529 (228)688-5846; DSN 828-5846 http://www.nrl.navy.mil <http://www.nrl.navy.mil/> From: Tony Craig <notifications@github.com> Sent: Friday, February 28, 2020 15:23 To: CICE-Consortium/CICE <CICE@noreply.github.com> Cc: David Hebert, Code 7322 <david.hebert@nrlssc.navy.mil>; Mention <mention@noreply.github.com> Subject: Re: [CICE-Consortium/CICE] Iceopen (#410) This looks good. I think we can merge it when testing is done. I would like to merge this today after the JRA55 PR. My only final comment is that if nbits/bits_per_byte does not divide evenly, the new computation could produce a different result. That is partly why I was suggesting integer*8, but I don't think we'll run into that issue. Keep us posted on test results. I hope to include this in the weekend testing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#410?email_source=notifications&email_token=AE52VPA7S662IFWOVJ2PT23RFF6E3A5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENKGYCI#issuecomment-592735241> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPHTVII2ZTM7MTRVN7TRFF6E3ANCNFSM4K5DLWLQ> . <https://github.com/notifications/beacon/AE52VPBMXHT7G4IPEXH74QLRFF6E3A5CNFSM4K5DLWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENKGYCI.gif>

[phil-blain]

@apcraig for this PR it would have been useful in my opinion to edit the commit message generated by the "Squash and Merge" button, because now the commit message mentions several commits that were not the subject of this PR, but were included because the branch iceopen had merge commits coming from merging the consortium master (and probably Dave's master also ? )...