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

Fixes an integer-kind mismatch in MOM_random, seed_from_time() #1113

Merged
merged 1 commit into from
May 19, 2020
Merged

Fixes an integer-kind mismatch in MOM_random, seed_from_time() #1113

merged 1 commit into from
May 19, 2020

Conversation

adcroft
Copy link
Collaborator

@adcroft adcroft commented May 19, 2020

  • gcc/8.3.0 issued Error: Integer too big for its kind reported in
    feedback on PR dev/master candidate 2020-05-15 #1111. The intent was to assume kind=4 in these
    calculations but apparently our compilers were promoting
    mod(dy + 32*(mo + 13*yr), 2147483648) to kind=8. There were two
    mistakes in the expression:
    • the use of 2147483648 in the mod is not representable with kind=4;
    • the mod produces negative values and should have been a modulo.
  • This commit reduces the range of the results by one number on the
    positive side and removes all the negatives.

@gustavo-marques Could you test this patch for me? It passes everything at our end but so did the original PR.

- gcc/8.3.0 issued `Error: Integer too big for its kind` reported in
  feedback on PR #1111. The intent was to assume kind=4 in these
  calculations but apparently our compilers were promoting
  `mod(dy + 32*(mo + 13*yr), 2147483648)` to kind=8. There were two
  mistakes in the expression:
  - the use of `2147483648` in the `mod` is not representable with kind=4;
  - the `mod` produces negative values and should have been a `modulo`.
- This commit reduces the range of the results by one number on the
  positive side and removes all the negatives.
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (dev-master-candidate-2020-05-15@8f57abf). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                        Coverage Diff                         @@
##             dev-master-candidate-2020-05-15    #1113   +/-   ##
==================================================================
  Coverage                                   ?   46.07%           
==================================================================
  Files                                      ?      214           
  Lines                                      ?    69379           
  Branches                                   ?        0           
==================================================================
  Hits                                       ?    31964           
  Misses                                     ?    37415           
  Partials                                   ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f57abf...fab21a7. Read the comment docs.

@marshallward
Copy link
Collaborator

I know this isn't the only place where this is being violated, but we really ought to stop assuming that kind is equal to the number of bytes. There is at least one major compiler (NAG) which does not make this assumption.

I'd recommend using iso_fortran_env, only : int4.

@gustavo-marques
Copy link
Collaborator

I can confirm it builds with gnu/8.3.0.

@adcroft adcroft merged commit 634991d into mom-ocean:dev-master-candidate-2020-05-15 May 19, 2020
@klindsay28
Copy link
Contributor

@marshallward , it looks like int4 was introduced in Fortran 2008.

If you want to support earlier Fortran versions, you can use i4 = selected_int_kind(6). This is what CIME and CESM components use. selected_int_kind was introduced in Fortran 90.

Does the MOM6 development team have a position on minimal Fortran version required for model development? I don't see this at https://mom6.readthedocs.io/en/dev-gfdl/index.html or https://github.com/NOAA-GFDL/MOM6/wiki.

@marshallward
Copy link
Collaborator

@klindsay28 I recall last we discussed this, we found FMS had already moved onto 2008, and so we've opted to do the same. (We're already using iso_fortran_env types elsewhere in the code).

I agree we need to clarify our target standard somewhere in the docs.

@adcroft adcroft deleted the dev-master-candidate-2020-05-15-patch1 branch June 15, 2020 19:12
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.

5 participants