Fix REAL(8) to REAL(4) conversion and tab warnings in f_alaska.f - #17
Open
d-diaz wants to merge 1 commit into
Open
Fix REAL(8) to REAL(4) conversion and tab warnings in f_alaska.f#17d-diaz wants to merge 1 commit into
d-diaz wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clears 40 of the 41 gfortran
-Wallwarnings inf_alaska.f(29-Wconversion, 11-Wtabs) by making the already-intended narrowing conversions explicit withREAL()and replacing hard tabs with spaces. No numerical change. Warning cleanup only.This follows the same pattern as #15 (
f_west.f) and #16 (f_other.f).The warning
All 29 conversion warnings are byte-identical apart from position:
plus 11 driver-level warnings:
Each conversion site stores a
REAL*8coefficient or intermediate into aREAL*4scalar. gfortran flags the implicit narrowing because it can silently lose precision. Here the narrowing is deliberate — the source says so atf_alaska.f:162, immediately above nine of the sites:c (These outcomes are SINGLE precision)The fix
One edit class: wrap the right-hand side in
REAL()at the point of assignment.Coefficient unpack (
VAR_AK, 16 sites of this shape):Logit block (
SHP_AK) — these six lines are byte-identical to theSHP_W*routines inf_west.f, so this is the same edit already proposed in #15:Function result (
COR_AK) — note the statement label in columns 1–5 is preserved:And each of the 11 tabs:
This is a provable no-op: the compiler already emitted exactly this conversion. The declarations are unchanged, so the stored value is the same
REAL(4)rounding of the sameREAL(8)expression —REAL()only states in source what the compiler was doing silently. The tab replacement is likewise exact: gfortran's fixed-form extension already advances a column-1 tab to column 7, so substituting 6 spaces leaves every effective column unchanged. The sweep below is confirmation.One site deserves a reviewer's attention.
SHP_AK:113-114andVAR_AK:393compute the same median-diameter intermediate asf_west.f:74-75andf_other.f:301-302,634-635— but those files declareDMEDIAN/DFORM/DRATIOasREAL*8(f_west.f:33,171,321,f_other.f:184,521), whilef_alaska.f:33,313declares themREAL*4. That difference is the entire reason these three lines warn here and the identical statements elsewhere do not. Widening the declarations to match the sibling files is arguably the correct fix and would improve accuracy, but it changes every downstreamU1–U9value, so it was not changed here — flagged below instead.What was deliberately not changed:
REAL*4→REAL*8) — that would change results.SHP_AK:114is not rewritten from1.0d0to1.0e0. That literal sits in an arithmetic expression, not aDATAinitializer, so it promotes the whole expression to double precision; respelling it would evaluate in single precision and can move results. Same trap asf_other.f:876in Fix REAL(8) to REAL(4) conversion warnings in f_other.f #16. It is wrapped inREAL()and left otherwise alone.Test evidence
On the development fork...
f_alaska.f41 → 1; no new warnings anywhereDetails:
JSP31–36 — Alaska yellow-cedar, western redcedar, and spruce/hemlock in both old-growth and second-growth form) crossed with a grid of dbh (6–45 in), total height (30–150 ft), merchantable tops, stump height, and upper-stem point including one below breast height. Every one of the 15vol[]components matched bit for bit, as diderrflag.Fcoefficient in its sixth decimal place (0.36073443D+01→0.36073543D+01) produced 539 mismatches, and they fell exactly in the two species-042 equations — the precise blast radius for that coefficient — confirming the comparison detects changes at the level being claimed.SHP_AKcoefficient sets and all sixFDBT_AKspecies branches across both the 2-point and 3-point entry paths, plus 6 that assert the Region 10 equation lookup returns the strings those cases use. A gcov build confirmed these cases execute all 29 edited lines — so the bit-identity result covers every line touched, not just the file in aggregate. Only one edited line is conditional (SHP_AK:168, underIF (U5 .le. 7.0d0)); it records 16 executions against 24 for its neighbours, so both branches are genuinely exercised.How to reproduce
With nothing but a stock gfortran, confirm the warnings are gone:
Before this change that prints 41 warnings; after, one remains —
Unused dummy argument 'setopt'inFDBT_AK. That argument is left in place deliberately:sf_shp.f:28dispatchesFDBT_AKthrough the same signature as the otherFDBT_*bark routines, so removing it would break the call. Happy to address it separately if you would prefer it gone.Additional context
Not required reading to review this PR:
warningsjob shows the 41 → 1 drop; thetestjob'sRun pyteststep shows the 18 newf_alaska_*andgetvoleq_r10_*cases.upstream/master.f_alaska.fthere is byte-identical to the file the fix was developed and tested against, so the diff below is exactly the change that was verified.Separate issues that should be considered for later resolution (not fixed here)
While working in this file I found issues whose fixes would each change numerical output or dispatch behavior. None belong in a no-op warning PR. Happy to open issues or follow-up PRs if useful:
SHP_AKcorrupts its own coefficient table.f_alaska.f:107-111patches theDATA-initialized (and therefore implicitlySAVEd)Farray in place whenGEOSUB=='01'for spruce/hemlock, and never restores it. The first such call overwritesF(25,3),F(34,3),F(42,3)for the life of the process, so every laterJSP33/34 call silently returns second-growth results regardless of its own geosub. Measured: after a singleA01F32W098call, a subsequentA00F32W098moves from 107.5 to 113.3 cuft (+5.4%) and 450 to 510 bdft (+13.3%), with two product fields dropping to zero. It appears currently unreachable through the shipped tables — the onlyA01equations involeqdef.fareA01BRUW202,A01DEMW000,A01DVEW094,A01DVEW375andA01DVEW747, none of which satisfyVOLEQ(4:4)=='F'— so this is latent rather than live bug.f_ingy.f:280-300solves the same problem correctly by copying into a scratch column that is rebuilt on every call. Worth asking: is the geosub-01second-growth submodel meant to be reachable at all? If it is obsolete, deleting the block may be better than fixing it.DMEDIAN/DFORM/DRATIOare narrower here than in the sibling files.f_alaska.f:33,313declares themREAL*4;f_west.f:33,171,321andf_other.f:184,521useREAL*8. Alaska therefore rounds the median-diameter intermediate to single precision and feeds that loss through the wholeU1–U9chain. This looks like an oversight rather than a decision.sf_shp.f:50uses.OR.where every sibling guard uses.AND.—ELSEIF(JSP.GE.31 .OR. JSP.LE.36)is unconditionally true. Comparesf_shp.f:26,sf_corr.f:25,sf_dfz.f:24,sf_3pt.f:116. It is last in theELSEIFchain so currently reachableJSPvalues still route correctly, but any value not caught earlier (1, 2, 6–10, 37+) falls intoSHP_AKand indexesF(10, JSP-30)out of bounds.f_alaska.f:130testsIF(JRSP .eq. 15)for a Lodgepole Pine (INGY) model, butJRSPis remapped to 1–4 at lines 101–104 and can never reach 15.f_alaska.f:3-6lists the file's routines asSHP_AK,COR_AK,VAR_AK— omittingFDBT_AK.