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

Convert logical to MASK_TYPE (aka integer*4) #7

Merged
merged 11 commits into from
Dec 18, 2024

Conversation

mabruzzo
Copy link

This PR should be reviewed after #6


Primary change: Convert logical to MASK_TYPE (aka integer*4)

This patch converts nearly all usages of the logical type to the newly defined MASK_TYPE in the Fortran source files throughout Grackle.

  • The MASK_TYPE type is a custom datatype that just wraps a 32bit integer.

    • Because Fortran will not implicitly cast between logical and integer we need to explicitly compare values stored in these variables against MASK_TRUE and MASK_FALSE.
    • For consistency with C semantics, if-statements that want to see if the mask is true always compare MASK_TYPE value is not equal to MASK_FALSE.
  • The only logical variable that wasn't changed was the end_int variable that we pass into interpolate_3Dz_g. We haven't touched this variable to avoid interfering with existing efforts to transcribe that routine to C/C++

  • This patch is extremely similar in spirit to GH PR Convert itmask from logical to integer*4 grackle-project/grackle#227. But there a few minor distinctions:

    • I am actually more confident in the correctness of this patch (I used custom tools to automate the majority of these changes)
    • I converted more variables in this patch (that other PR only changed the itmask variable).
  • I also needed to add #include "grackle_fortran_types.def" into the body of calc_grain_size_increment_species_1d1

Other Changes

There were 2 other minor tweaks that I made:

  1. I slightly refactored some ifdef statements within cool1d_multi_g so that my utility scripts would find it easier to understand2
  2. I fixed a minor issue where we had defined some internally used arrays that are declared as R_PREC within solve_rate_cool_g but then declared them as REAL*8 in step_rate_g (I simply replaced REAL*8 with R_PREC)

Note

I have manually confirmed that all tests pass when you include the commits in this PR

As noted in #4, the test_models.py tests are a little flaky (this is a train of the gen2024 branch without any of my changes). The reason I didn't strip this PR down to just the 2 commits described in the Primary change section is that I was having a hard time getting to pass with just those PRs (they seem to pass more readily when I include all 6 commits). With that said these other changes are totally independent of the primary changes.

Footnotes

  1. Originally, I made this change purely for the sake of consistency, but it's actually a necessary change since that statement defines the MASK_KIND constant and the MASK_KIND constant is used within the definition of MASK_TRUE and MASK_FALSE.

  2. There is a chance this will slightly slow things down, but it would be a very minor performance impact. I think it also makes the code a lot easier to read and understand. (Plus I think that most compilers should be able to avoid the extra branch that I effectively stuck into the codebase)

Previously, there were 2 different ways to declare parameters scattered
throughout the codebase. This patch standardizes on a single approach
that is easier to transcribe.
This patch converts nearly all usages of the ``logical`` type to the
newly defined ``MASK_TYPE`` in the Fortran source files throughout
Grackle.
- The ``MASK_TYPE`` type is a custom datatype that just wraps a 32bit
  integer.

  - Because Fortran will not implicitly cast between logicals and
    integers we need to explicitly compare this value against
    ``MASK_TRUE`` and ``MASK_FALSE``.
  - For consistency with C semantics, if-statements that want to see
    if the mask is true always compare ``MASK_TYPE`` value is not equal
    to ``MASK_FALSE``.

- The **only** ``logical`` variable that wasn't changed was the
  ``end_int`` variable that we pass into ``interpolate_3Dz_g``.
  We haven't touched this variable to avoid interfering with existing
  efforts to transcribe that function to C/C++

- This patch is extremely similar in spirit to GH PR
  grackle-project#227. But there a fewer minor distinctions:

  - I am actually more confident in the correctness of this patch (I used
    custom tools to automate the majority of these changes)
  - I converted more variables in this patch (that other PR **only**
    changed the ``itmask`` variable).

I have explicitly confirmed that all tests continue to pass after the
introduction of this PR
The `cool1d_multi_g` subroutine expects an argument `iter`. Previously,
the `cool_multi_time_g` subroutine would pass a value of 1 directly to
this argument.

To simplify the process of transcription, we now store the value inside
of a local variable called `dummy_iter_arg` (if we didn't do this, we
would need to insert logic into our transcription routine to inject
a custom variable to hold the value of 1 and then pass a pointer to that
value into `cool1d_multi_g`).
I made some minor tweaks to where we pass in the pointers to the
i-dimension of grid_dimensions, grid_start, and grid_end. This is a
purely superficial change, but will allow automated transcription to
produce better code.
Comment on lines -516 to +551
#ifdef CALCULATE_TGAS_SELF_CONSISTENTLY

iter_tgas = 0
tgas_err = huge8
do while ((iter_tgas .lt. 100)
& .and.(tgas_err .gt. 1.d-3))
tgas0 = tgas(i)
#endif
if (nH2/nother .gt. 1.0e-3_DKIND) then
x = 6100._DKIND/tgas(i) ! not quite self-consistent
if (x .gt. 10._DKIND) then
gamma2 = 0.5_DKIND*5._DKIND
tgas0 = tgas(i)
if (nH2/nother .gt. 1.0e-3_DKIND) then
x = 6100._DKIND/tgas(i) ! not quite self-consistent
if (x .gt. 10._DKIND) then
gamma2 = 0.5_DKIND*5._DKIND
else
gamma2 = 0.5_DKIND*(5._DKIND + 2._DKIND*x**2
& * exp(x)/(exp(x)-1)**2)
endif
else
gamma2 = 0.5_DKIND*(5._DKIND + 2._DKIND*x**2 *
& exp(x)/(exp(x)-1)**2)
gamma2 = 2.5_DKIND
endif
else
gamma2 = 2.5_DKIND
endif
gamma2 = 1._DKIND + (nH2 + nother)/
& (nH2*gamma2 + nother/(gamma-1._DKIND))
gamma2 = 1._DKIND + (nH2 + nother)/
& (nH2*gamma2 + nother/(gamma-1._DKIND))
#ifdef CALCULATE_TGAS_SELF_CONSISTENTLY
tgas(i) = max((gamma2 - 1._DKIND)*mmw(i)*e(i,j,k)*utem
& , temstart)
tgas_err = dabs(tgas0 - tgas(i)) / tgas0
iter_tgas = iter_tgas + 1
end do
tgas(i) = max((gamma2 - 1._DKIND)*mmw(i)*e(i,j,k)*
& utem, temstart)
tgas_err = dabs(tgas0 - tgas(i)) / tgas0
iter_tgas = iter_tgas + 1
#else
tgas(i) = tgas(i) * (gamma2 - 1._DKIND)/
& (gamma - 1._DKIND)
tgas(i) = tgas(i) * (gamma2 - 1._DKIND)/
& (gamma - 1._DKIND)
iter_tgas = 101
#endif
end do
Copy link
Owner

Choose a reason for hiding this comment

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

This is hard to parse just looking at the diff. Is this just fixing the tab level? Does the ifdef still cover this whole block?

Copy link
Author

Choose a reason for hiding this comment

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

Originally the ifdef did not cover the whole block.

  • The do while and end do statements were conditionally included
  • The change I made here was to always include the do while and end do statements
  • This is needed for the automatic transcription logic

I have attempted to show what the code looked like before and after the change (hopefully I didn't mess up my copy/pasting from the diff).

Here is the original

#ifdef CALCULATE_TGAS_SELF_CONSISTENTLY
                  iter_tgas = 0
                  tgas_err = huge8
                  do while ((iter_tgas .lt. 100)
     &                 .and.(tgas_err .gt. 1.d-3))
                  tgas0 = tgas(i)
#endif
                  if (nH2/nother .gt. 1.0e-3_DKIND) then
                     x = 6100._DKIND/tgas(i) ! not quite self-consistent
                     if (x .gt. 10._DKIND) then
                        gamma2 = 0.5_DKIND*5._DKIND
                     else
                        gamma2 = 0.5_DKIND*(5._DKIND + 2._DKIND*x**2 * 
     &                       exp(x)/(exp(x)-1)**2)
                     endif
                  else
                     gamma2 = 2.5_DKIND
                  endif
                  gamma2 = 1._DKIND + (nH2 + nother)/
     &                 (nH2*gamma2 + nother/(gamma-1._DKIND))
ifdef CALCULATE_TGAS_SELF_CONSISTENTLY
                  tgas(i) = max((gamma2 - 1._DKIND)*mmw(i)*e(i,j,k)*utem
     &                        , temstart)
                  tgas_err = dabs(tgas0 - tgas(i)) / tgas0
                  iter_tgas = iter_tgas + 1
                  end do
#else
                  tgas(i) = tgas(i) * (gamma2 - 1._DKIND)/
     &                 (gamma - 1._DKIND)
#endif

After the changes, the block now looks like:

                  iter_tgas = 0
                  tgas_err = huge8
                  do while ((iter_tgas .lt. 100)
     &                 .and.(tgas_err .gt. 1.d-3))
                     tgas0 = tgas(i)
                     if (nH2/nother .gt. 1.0e-3_DKIND) then
                        x = 6100._DKIND/tgas(i) ! not quite self-consistent
                        if (x .gt. 10._DKIND) then
                           gamma2 = 0.5_DKIND*5._DKIND
                        else
                           gamma2 = 0.5_DKIND*(5._DKIND + 2._DKIND*x**2
     &                       * exp(x)/(exp(x)-1)**2)
                        endif
                     else
                        gamma2 = 2.5_DKIND
                     endif
                     gamma2 = 1._DKIND + (nH2 + nother)/
     &                    (nH2*gamma2 + nother/(gamma-1._DKIND))
#ifdef CALCULATE_TGAS_SELF_CONSISTENTLY
                     tgas(i) = max((gamma2 - 1._DKIND)*mmw(i)*e(i,j,k)*
     &                    utem, temstart)
                     tgas_err = dabs(tgas0 - tgas(i)) / tgas0
                     iter_tgas = iter_tgas + 1
#else
                     tgas(i) = tgas(i) * (gamma2 - 1._DKIND)/
     &                    (gamma - 1._DKIND)
                     iter_tgas = 101
#endif
                  end do

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, I was concerned that the inner bit might have been needed elsewhere, but it's only populating the scalar gamma2, so clearly not. This is fine. Probably something else to refactor or turn into a runtime parameter.

Copy link
Owner

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I just had the one question about cool1d_multi. The rest looks fine.

@brittonsmith brittonsmith merged commit 185635a into brittonsmith:gen2024 Dec 18, 2024
@mabruzzo mabruzzo deleted the gen2024-dev branch December 20, 2024 15:26
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.

2 participants