Case insensitive F90 compile rule #2057

MicroTed · 2024-06-03T19:47:58Z

The .F90 source files in physics_mmm need to be compiled directly to .o on case-insensitive file systems (like MacOS standard FS). This PR avoids the creation of a .f90 temporary, which will clobber the .F90 version.

TYPE: [bug fix]

KEYWORDS: case-insensitive filesystem

SOURCE: "Ted Mansell (NOAA/NSSL)"

DESCRIPTION OF CHANGES:
Problem:

On case-insensitive file systems (like MacOS standard FS), the creation of .f90 temporary files from .F90 source will clobber the source files.
The configure.defaults has an unrecognized option for Apple clang

Solution:

In postamble: Removed the .F90.f90 rule and compacted the .F90.o rule to use only the fortran compiler to do any necessary preprocessing. It is convention among fortran compilers that .F90 will be preprocessed.
In configure.defaults: For Darwin compile with ifort+clang, removed the -qopenmp from OMPCC (Apple clang does not support OpenMP)

ISSUE: Addresses .F90.f90 issue raised in #1989

LIST OF MODIFIED FILES:
arch/postamble
arch/configure.defaults

TESTS CONDUCTED:

Compiles correctly on MacOS (Intel CPU with ifort + clang) and Linux
Are the Jenkins tests all passing? Yes.

RELEASE NOTE: Fixed a compile problem with .F90 source files on case-insensitive file systems.

Pleim-Xiu LSM MODIS LCZ Compatibility & Surface Evaporation Update TYPE: bug fix & physics refinement KEYWORDS: MODIS, LCZ, P-X LSM, Latent Heat Flux SOURCE: Robert Gilliam & Jon Pleim, US EPA DESCRIPTION OF CHANGES: Problem: User indicated that the P-X LSM errored for MODIS LCZ 61 NUM_LAND_CAT configuration. Currently, we account for evaporation from transpiration, soil in both vegetated and non-veg parts, and wet leaves. But we only account for the latent heat effects on Tg from transpiration and evaporation from non-veg soil. This fix adds latent heat effects on Tg from soil in vegetated parts and from wet leaves. Fix for rare case where GRDFLX goes NaN because of a divide by zero based on a soil parameter when a water cell turns to sea ice. Solution: Logic checks in module_physics_init.F and module_sf_pxlsm.F were adjusted for 61 category inputs. P-X LSM data table, module_sf_pxlsm_data.F was updated for MODIS 61 categories. Default for LCZ 51-61 was set to MODIS urban class. We also added updates for the evaporation from vegetation and wet canopy. ISSUE: Fixes: wrf-model#1965 LIST OF MODIFIED FILES: M phys/module_physics_init.F M phys/module_sf_pxlsm.F M phys/module_sf_pxlsm_data.F - Tested 61 class LCZ with PX LSM for a 1 day simulation with updated codes. Ran base MODIS 21 class scheme with same code before and after LCZ update. The results were identical after a 24 hour simulation. This confirms updates do not impact other MODIS settings in the P-X LSM. The MODIS 21 was by nature not identical to the MODIS 61, but similar enough and differences follow underlying differences in MODIS datasets. - The Jenkins tests are all passing. RELEASE NOTE: Pleim-Xiu LSM is now compatible with 61 category MODIS LCZ landuse dataset. A mode of latent heat effects on Tg from vegetated parts and from wet leaves is added to Pleim-Xiu LSM.

…lang because Apple clang does not support it arch/postamble : Simplified .F90.o to remove creation of the .f90 intermediary because it clobbers the .F90 on case-insensitive file systems (Mac)

weiwangncar · 2024-06-03T20:47:17Z

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

mgduda · 2024-09-10T23:35:05Z

arch/configure.defaults

@@ -915,7 +915,7 @@ DESCRIPTION     =       INTEL ($SFC/$SCC)
 DMPARALLEL      =       # 1
 OMPCPP          =       # -D_OPENMP
 OMP             =       # -qopenmp -fpp -auto
-OMPCC           =       # -qopenmp
+OMPCC           =       #


I may be mistaken, but I think clang does support OpenMP on Apple systems, though the flag is -fopenmp rather than -qopenmp (which may have simply been copied from the Intel Fortran compiler option to enable OpenMP).

The default built-in Xcode clang does not support the -fopenmp flag (throws an error). Users can install clang (or gcc) with homebrew, and that does support it, but I would think that users would not expect to have to do that. Is there much C code in the model that uses openmp, anyway?

Those are fair points, and I don't think there's much C code in WRF that uses OpenMP. Especially since this change only affects Intel-based macOS systems, I'm fine with the change as it is.

I'll let others (@weiwangncar, @islas) weigh in on whether this change should appear in a separate PR or commit since it doesn't strictly relate to case-insensitive *.F90 compilation.

Apple clang doesn't support openmp on either intel or arm64 macs. (It is possible to run ifort on an M-series mac in emulation, and it actually runs OK.) I didn't think to update the gfortran+clang section since I've never used it.

I would prefer the arch/configure.defaults changes to be separate, as that would also make recording / release notes easier to write each up as separate fixes

mgduda · 2024-09-10T23:49:20Z

arch/postamble

-	$(SED_FTN) $*.bb | $(CPP) $(TRADFLAG) > $*.f90
-	$(RM) $*.G $*.bb
-	$(FC) -o $@ -c $(FCFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.f90
+	$(FC) -o $@ -c -I$(WRF_SRC_ROOT_DIR)/inc $(OMPCPP) $(FCFLAGS) $(ARCHFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.F90


Under the assumption that there is a good reason for explicitly processing source files with cpp and sed, would it be safer to preserve that preprocessing, but to instead name the intermediate .f90 files something like .tmp.f90?
I think something like this might be possible (pending testing with a broader range of compilers):

.F90.o: $(RM) $@ sed -e "s/^\!.*'.*//" -e "s/^ *\!.*'.*//" $*.F90 > $*.G $(CPP) -I$(WRF_SRC_ROOT_DIR)/inc $(CPPFLAGS) $(OMPCPP) $*.G > $*.bb $(SED_FTN) $*.bb | $(CPP) $(TRADFLAG) > $*.tmp.f90 $(RM) $*.G $*.bb $(FC) -o $@ -c $(FCFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.tmp.f90

I don't know the history of WRF compiling to use the cpp/sed etc., but it seems like a lot of extra effort when the built-in fpp should just work. (Maybe the old cray compilers were cranky?) A mangled name should work, too, though, if that would be preferable. I just thought it would be much simpler to just compile it straight. I have a bunch of preprocessing in my codes and haven't run into any problems with built-in fpp, but I also haven't used all possible fortran compilers.

The 'clean' function is the only thing that might need to be updated?

My preference would be to stick with what has been known to work for WRF. In MPAS, we do let the Fortran compiler handle pre-processing of source files by default, but I seem to recall that WRF code contains some non-standard-isms that necessitated special pre-processing for some compilers.

I don't think anything needs to be modified for the 'clean' script, as that appears to have logic already to remove .f90 files where they are know to have been generated through explicit pre-processing: https://github.com/wrf-model/WRF/blob/v4.6.0/clean#L6-L16 .

That's fair. The current codes in physics_mmm have hardly any preprocessing, but there's no guarantee of that always being the case.

MicroTed · 2024-10-08T18:37:37Z

Sorry it took so long, but I reverted configure.defaults so that it can be a separate thing.

…rary file

mgduda · 2024-10-11T21:38:56Z

@islas @weiwangncar Should we target the release-v4.6.1 branch with this PR?

islas · 2024-10-11T22:02:04Z

@mgduda I think that should work especially with the more conservative approach of tmp suffix files.

The base branch was changed.

coastwx and others added 3 commits March 27, 2024 16:11

Merge branch 'master' of github.com:wrf-model/WRF into develop

754f0e5

arch/configure.defaults : Removed -qopenmp from OMPCC for Mac with c…

2948495

…lang because Apple clang does not support it arch/postamble : Simplified .F90.o to remove creation of the .f90 intermediary because it clobbers the .F90 on case-insensitive file systems (Mac)

MicroTed requested a review from a team as a code owner June 3, 2024 19:47

weiwangncar changed the base branch from master to develop June 3, 2024 20:46

weiwangncar added the Develop Branch label Aug 6, 2024

islas mentioned this pull request Sep 6, 2024

Breakout deallocation calls into simpler smaller files #2070

Merged

mgduda requested changes Sep 11, 2024

View reviewed changes

mgduda self-requested a review September 11, 2024 21:14

Reverted configure.defaults to be able to make a separate PR with that.

afb7c89

Changed .F90 rule to use WRF default for .F with '.tmp.f90' as tempo…

46475a4

…rary file

mgduda previously approved these changes Oct 11, 2024

View reviewed changes

islas changed the base branch from develop to release-v4.6.1 October 11, 2024 22:02

mgduda self-requested a review October 11, 2024 22:40

mgduda approved these changes Oct 11, 2024

View reviewed changes

islas approved these changes Oct 11, 2024

View reviewed changes

islas merged commit 6e71a0a into wrf-model:release-v4.6.1 Oct 11, 2024
1 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Case insensitive F90 compile rule #2057

Case insensitive F90 compile rule #2057

MicroTed commented Jun 3, 2024 •

edited

Loading

weiwangncar commented Jun 3, 2024

mgduda Sep 10, 2024

MicroTed Sep 11, 2024

mgduda Sep 11, 2024

mgduda Sep 11, 2024

MicroTed Sep 11, 2024

islas Sep 13, 2024

mgduda Sep 10, 2024

MicroTed Sep 11, 2024

mgduda Sep 11, 2024

MicroTed Sep 11, 2024

MicroTed commented Oct 8, 2024

mgduda commented Oct 11, 2024

islas commented Oct 11, 2024

Case insensitive F90 compile rule #2057

Case insensitive F90 compile rule #2057

Conversation

MicroTed commented Jun 3, 2024 • edited Loading

weiwangncar commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroTed commented Oct 8, 2024

mgduda commented Oct 11, 2024

islas commented Oct 11, 2024

MicroTed commented Jun 3, 2024 •

edited

Loading