Inconsistent floating point types in quickbeam_optics

[brhillman]

Different conventions for floating point literals in quickbeam_optics.F90 lead to compile-time errors when using the intel compiler with the -r8 flag. For example:

../subsample_and_optics_example/optics/quickbeam_optics/quickbeam_optics.F90(472): error #6633: The type of the actual argument differs from the type of the dummy argument.
          ld   = ((apm*gamma(1.+bpm)*N0)/(rho_a*Q*1E-3))**tmp1

The problem appears to be because bpm is declared properly as a real(wp), but this conflicts with the precision of the 1. literal when the -r8 flag is used. We could omit the -r8 flag, but this seems to be required by other parts of our model. The bigger issue seems to be inconsistent floating point literals in the code here, with some instances using gamma(1+bpm) and others gamma(1.+bpm). My understanding is that adding the decimal without giving an explicit precision is somewhat dangerous, because we can't be sure what precision will be used. A safer approach (and one that resolves the above compile-time error) would be to either drop the decimal from each instance in the code, or else be explicit with precision by appending _wp to each literal, like gamma(1._wp+bpm). I can confirm that either of these fixes resolve the above error.

[RobertPincus]

@brhillman you are really the best. Would it be too much to ask you to fork the repo, make the changes, and open a PR? I'd favor the 1._wp form of the fix.

[dustinswales]

@brhillman Thanks for this. I second Robert's opinion.

Also @RobertPincus, since this is change doesn't impact the core COSP code and instead lives in subsample_and_optics_example/, do we need to go through the formal review process?

[RobertPincus]

@dustinswales I would think not but we could ask the PMC. This fix is very specific and won't change answers under most circumstances.

[brhillman]

@RobertPincus no problem, are there tests I need to run? Or do ya'll run those after I submit the PR?

[alejandrobodas]

@dustinswales , @RobertPincus , why wouldn't we want to follow the standard review process? The code review for this change is trivial, so the only thing we need to check is that the change doesn't modify the standard outputs. If it does, then we need to update the known good outputs (KGOs). I think the issue of whether the code in subsample_and_optics_example is part of COSP or not is not relevant, since modifications to subsample_and_optics_example have the potential of changing the KGOs, which would create problems when testing future modifications.

[RobertPincus]

@alejandrobodas Yes, someone (Dustin, I guess, or we could ask @brhillman) should run the regression tests to be sure the answers don't change. They won't, I'm confident. Do we then want to require further review? It seems like a lot of process for something so trivial.

[alejandrobodas]

@RobertPincus That sounds good to me. I just wanted to highlight that, no matter how confident we are that a modification won't change results, we must run the regression tests and copy&paste the results in the PR or issue so that it gets properly documented.

[brhillman]

@RobertPincus @alejandrobodas I ran the regression tests on the bugfix branch, but I am getting diffs in unrelated fields. Thinking maybe the reference data was stale, I tried again using master and get the same result. Are the references for master maybe old? Here are the diffs (and command used to compare):

(python2) [bhillma@ghost-login2 driver (master)]$ python test_cosp2Imp.py data/outputs/UKMO/cosp2_output_um.ref.nc data/outputs/UKMO/cosp2_output_um.nc
############################################################################################
Treating relative differences less than 0.0010000000% as insignificant
  atb532_perp:         16.30 % of values differ, relative range:  -5.90e-05 to  7.03e-05
  atb532:              17.08 % of values differ, relative range:  -5.89e-05 to  7.02e-05
  cfadLidarsr532:       0.00 % of values differ, relative range:  -1.00e+00 to  1.00e+00
  lidarBetaMol532:     19.45 % of values differ, relative range:  -5.91e-05 to  7.02e-05
  clthinemis:           0.65 % of values differ, relative range:  -1.18e-05 to -1.18e-05
  lidarBetaMol532gr:     18.56 % of values differ, relative range:  -5.89e-05 to  6.97e-05
  atb532gr:            13.87 % of values differ, relative range:  -5.89e-05 to  6.97e-05
  lidarBetaMol355:      3.99 % of values differ, relative range:  -1.53e-05 to  2.19e-05
  atb355:               3.74 % of values differ, relative range:  -1.53e-05 to  2.22e-05
  dbze94:               0.24 % of values differ, relative range:  -2.78e-04 to  1.05e-04
  boxtauisccp:          1.93 % of values differ, relative range:  -2.37e-05 to  2.40e-05
  boxptopisccp:         2.22 % of values differ, relative range:  -4.67e-05 to  4.10e-05
All other fields match.

Most of these look small, but some do not (i.e., cfadLidarsr532, although it reports 0% of them differ?). Again, this happens with master too.

[alejandrobodas]

Hi @brhillman I think this is similar to the issue we had in #15. I suspect that your compiler/options/architecture introduces some differences. Please, can you update your reference files with the ones produced with master, and then run the regression tests with your modifications?

[brhillman]

Thanks @alejandrobodas that's exactly it. I had done this experiment but neglected to return the compiler flags back to default before doing that (I had inserted the flag to promote to r8 precision in one of the test but not the other). Rebuilding both the new branch and master with the identical flags and comparing produces identical results:

(python2) [bhillma@ghost-login2 driver (fix-quickbeam-optics-types)]$ python test_cosp2Imp.py ../../master/driver/data/outputs/UKMO/cosp2_output_um.nc data/outputs/UKMO/cosp2_output_um.nc
############################################################################################
Treating relative differences less than 0.0010000000% as insignificant
All fields match
(python2) [bhillma@ghost-login2 driver (fix-quickbeam-optics-types)]$