Modifs to genmake2 and adjoint_default to enable mixed .f .f90 with TAF #709
Conversation
* changes to genmake2 to enable mixed .F and .F90 code for TAF
|
Hi Patrick, I always wanted to to be able to use both *.F90 and F. files. the taf output in make.tr_log starts with and I tried Here, TAF does not seem to be called at all leading to a missing |
There was a problem hiding this comment.
Not a complete review and maybe just notes about the changes. I tried to run testreport -adm -t isomip again on a linux box, where it works, but not with -ncad. So two issues:
-ncaddoes not work (for me)- this does not work on MacOS (I assume shell tools are the problem)
I will try to find a fix for MacOS later. In the meantime, does it make sense to have an example with mixed .F and .F90 files in the verification experiments?
tools/genmake2
Outdated
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_F90FILES:.$FS90=_ad.$FS90) | ||
| @-rm -f \$(AD_F90FILES:.$FS90=_ad.$FS90.bak) |
There was a problem hiding this comment.
do we need the same for target ftl_sed (next couple of lines)?
tools/genmake2
Outdated
| \$(MAKE) -f \$(MAKEFILE) remove_comments | ||
| \$(TAF) \$(AD_TAF_FLAGS) \$(TAF_EXTRA) \$(FLOWFILES) \$(AD_FILES) | ||
| \$(TAF) \$(AD_TAF_FLAGS) \$(TAF_EXTRA) all_flowfiles.$FS \$(AD_FILES) \$(AD_F90FILES) |
There was a problem hiding this comment.
I assume that this line has to do with my failing compilation.
| @@ -1817,7 +1817,7 @@ if test "x${AD_OPTFILE}" != xNONE ; then | |||
| exit 1 | |||
| fi | |||
| fi | |||
| if test $CAT_SRC_FOR_TAF = 0 ; then TAF_EXTRA="${TAF_EXTRA} -fixed" ; fi | |||
| ### if test $CAT_SRC_FOR_TAF = 0 ; then TAF_EXTRA="${TAF_EXTRA} -fixed" ; fi | |||
There was a problem hiding this comment.
Why is this necessary? I thought (and I may be wrong) that our f90 files all need to be fixed format anyway.
When I add a -fixed to the taf-command, then at least the code without -nocat4ad/-ncad compiles and the isomip experiment runs. (but the -ncad test still does not work)
When I put back this line, I get no complaint from taf for testreport -adm -ncad, but a problem with target adj_sed (which may be related to the sed version on my laptop, which does not allow an empty list of input files $(AD_F90FILES) ).
There was a problem hiding this comment.
I thought this line had to be commented out to allow for free format code to be passed to TAF. The -fixed flag is the fortran compiler flag to enforce fixed-form.
|
With these changes, it seems to work for me also with MacOS. @heimbach do you want me to push this, or would you rather try this first. |
|
Martin,
Very quick responses between meetings:
I may misunderstand you, but we need to omit the “-fixed” option to be able to differentiate “true” (i.e., free-format) .F90 files. So for true .F90 files, “-fixed” must not be used. I also am not sure why this is needed at all, but probably simply can’t remember the reason.
As far as I can tell, the mixed .f, .f90 case can ONLY work with the “-ncad” option. In this way, TAF will automatically treat .f files as fixed format and .f90 files as free-format files (as it should). Not using “-ncad” would require concatenating .f90 files into a separate ad_input_code.f90 for TAF to treat those files properly.
The changes I made to genmake2 are not sufficient to run the mixed format case. For the test I used (more below), I added a genmake_local file in which I add two more changes:
1/ add TAF option “-e” in adoptfile
2/ add two compiler options in optfile that extend lines to 132 (this one may ultimately not be needed, but worked for TAF-generated code)
I have not tested on MacOS, only linux box. For item 2/ above I only initially added the gfortran compiler option. Will be good to make it generic.
I agree we should develop a verification for this ASAP. What I tested is not yet fully generic but getting there.
Here’s what I tested (on linux box, with gfortran):
https://github.com/christophernhill/gmao_mitgcm_couplng/tree/master/geos_bulkformulae/global_ocean.cs32x15
The code compiled successfully (including TAF) for the following steps (from global_ocean.cs32x15):
cd build_geos_ad
../../../tools/genmake2 -ncad -mods '../geos_blkf ../code_geos_ad'
make depend
make adtaf
make adall
Notice the genmake_local file in build_geos_ad/ to change options in adjoint_default
Also note that some flowdirs/keys in the geos_bulkf code are still incorrect (haven’t sorted the correct keys yet).
Another important change was that we need to now treat all flowdir files as .f files (since they are in F77 format), i.e., you’ll now see these lines in genmake2
cat \$(FLOWFILES) > all_flowfiles.$FS
\$(TAF) \$(AD_TAF_FLAGS) \$(TAF_EXTRA) all_flowfiles.$FS \$(AD_FILES) \$(AD_F90FILES)
I ran testreport on lab_sea with the modified genmake2 and it worked for me (that was with the default, which I guess does not use -nocad). Quite likely that I missed something though. I’ll be happy to test isomip later and see why it does not work.
Does that help?
p.
|
|
PS: Two more notes:
Perhaps prudent in the below test case to specify gfortran explicitly, ie
../../../tools/genmake2 -fc gfortran -ncad -mods '../geos_blkf ../code_geos_ad'
(to re-emphasize, the change to FFLAGS and F90FLAGS in genmake_local may not ultimately be needed, can explain later why they were needed here).
The relevant mix of .F and F.90 codes used in the test is in
https://github.com/christophernhill/gmao_mitgcm_couplng/tree/master/geos_bulkformulae/global_ocean.cs32x15/geos_blkf
|
|
PS2:
@mlosch The lines I added in genmake2
sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_F90FILES:.$FS90=_ad.$FS90)
@-rm -f \$(AD_F90FILES:.$FS90=_ad.$FS90.bak)
are not needed and will cause adjoint verifications with pure F77 code to fail when using the -ncad flag.
Removing those two lines should fix your isomip AD test with -ncad.
|
|
@heimbach I took the liberty to edit your posts (stripped the original messages that the emails replied to) to make it easier to follow this thread. Thanks for the explanations, I now understand more of the changes. Is still work in progress, or is this already ready for review? If it is work in progress, we should add the corresponding label or make it a draft PR and I will hold off any further amateur attempts to make this work for me (it seems that Otherwise, I am not sure what to do here. The first thing I normally do is check if this PR breaks anything. I just used isomip, because it is usually fast to process (for taf), but the problem is the same for all other experiments. Also with lab_sea, the target |
|
Quick status with not much time to follow through:
It is work in progress, but the changes to genmake2 ought to be foundational to make the rest work and should be maintained – once they work.
I do now see the errors that you are getting, a bit of a mystery. What is the case:
1/ ALL the TAF-generated _ad.f files are identical between original and modified genmake2 (which is good, and should be)
2/ oddly, where things differ is in the files that TAF does not see (e.g., the_model_main.F, ctrl_init_ctrlvar.f , and others) and where there is an ALLOW_ADJOINT_RUN CPP option. That option is not properly evaluated in these files, and which causes the errors due to missing code (e.g., all the adxx_ files are no longer initialized because ctrl_init_ctrlvar.f missed that code).
I don’t see yet how this can happen given my modifs (ALLOW_ADJOINT_RUN is set in AD_CONFIG.h, which has the correct #define).
The good news,
* it is probably something dumb I did that may have an easy fix
* all _ad.f files are still correct
|
|
I am also getting correct
for 2.: couldn't we just modify an existing experiment and add a new free-form routine that is called from a local file like In the meantime I'll add the work-in-progress label. |
mjlosch
added
the
work in progress
|
I think I have a solution to the first issue (testreport experiments failing). Basically the same as for the target in without any files and this will then make the target Also I think I know now why stuff is failing on the Mac: The filesystem does not differentiate between upper and lower case, so that the file suffixes $FS and $FS90 do not expand into |
tools/genmake2
Outdated
| @\$(MAKE) -f \$(MAKEFILE) \$(F77_PP_SRC_FILES) | ||
| @\$(MAKE) -f \$(MAKEFILE) \$(F90_PP_SRC_FILES) | ||
| @\$(MAKE) -f \$(MAKEFILE) \$(FLOWFILES) | ||
| cat \$(FLOWFILES) > all_flowfiles.$FS | ||
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS) \$(AD_FILES:.$FS=_ad.o); echo '' |
There was a problem hiding this comment.
Here, we do not need the FLOWFILES, and we could just use AD_FLOW_FILES, as the links are concatenated into a local file anyway. And, more importantly, we should combine the two "PP" lines, as shown below, so that F90_PP_SRC_FILES can be empty without problems:
| @\$(MAKE) -f \$(MAKEFILE) \$(F77_PP_SRC_FILES) | |
| @\$(MAKE) -f \$(MAKEFILE) \$(F90_PP_SRC_FILES) | |
| @\$(MAKE) -f \$(MAKEFILE) \$(FLOWFILES) | |
| cat \$(FLOWFILES) > all_flowfiles.$FS | |
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS) \$(AD_FILES:.$FS=_ad.o); echo '' | |
| @\$(MAKE) -f \$(MAKEFILE) \$(F77_PP_SRC_FILES) \$(F90_PP_SRC_FILES) | |
| cat \$(AD_FLOW_FILES) > all_flowfiles.$FS | |
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS) \$(AD_FILES:.$FS=_ad.o); echo '' |
tools/genmake2
Outdated
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_FILES:.$FS=_ad.$FS) | ||
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS.bak) | ||
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_F90FILES:.$FS90=_ad.$FS90) | ||
| @-rm -f \$(AD_F90FILES:.$FS90=_ad.$FS90.bak) |
There was a problem hiding this comment.
I think this is necessary if AD_F90FILES is empty (which is the case most of the time)
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_FILES:.$FS=_ad.$FS) | |
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS.bak) | |
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_F90FILES:.$FS90=_ad.$FS90) | |
| @-rm -f \$(AD_F90FILES:.$FS90=_ad.$FS90.bak) | |
| sed -i.bak -f \$(TOOLSDIR)/adjoint_sed \$(AD_FILES:.$FS=_ad.$FS) \$(AD_F90FILES:.$FS90=_ad.$FS90) | |
| @-rm -f \$(AD_FILES:.$FS=_ad.$FS.bak) \$(AD_F90FILES:.$FS90=_ad.$FS90.bak) |
|
with the TAF 5.8.0 we can now specify both fixed and free format files with arbitrary suffixes like this This solves the MacOS issue and it is no longer necessary concatenate all *.flow files into a new files with suffix @heimbach I can push a solution that contains all my previous suggestions, modified to work with TAF 5.8.0 for you to try. Let me know. |
- on MacOS, file systems are case-insensitive by default; we solve that by using different file extensions for preprocessed files (.for and .fr9); with TAF version 5.8.0 and higher, we can specify both fixed and free format files at the command line with appropriate flags; this also require an update of the staf script - add more lines to remove from preprocess files before sending them to taf.
|
@heimbach please try my latest commit. You need to install the latest staf script for this ( |
|
@mjlosch i'm going to try this... please stay tuned for some questions :-) |
|
Hi all, Thanks @mjlosch for pointing me to this PR!
It looks like the new |
|
@IvanaEscobar I made the comment you are referring to on Mar 10, 2023. I have committed fixes since (e37d726). It would be great if you could try them. |
|
I've updated Ran: Results: My changes are in a forked branch of MITgcm, In that branch there are some potentially irrelevant contributions:
These modifications do not cause my targeted testreport to fail. Can specify adjoint options files in testreport |
|
@IvanaEscobar It's great that it works for you, thanks for testing. About your branch genmake2_hybrid: I quickly looked at the differences with this branch and have these comments:
I have this suggestion: |
|
@mjlosch, I've checked on your points and address each one below:
The changes in my branch are identical to this PR.
Changing to
The
Leaving adoptfile as it's done in This seems to be
Is this a
This PR works for me, so let's hope it works for @antnguyen13 as well. I can set-up a separate PR for |
|
I did not follow the details, so just a side comment about a new testreport option: |
I hope that I am getting this right: in fwd genmake2 MacOS you modified this line 3258 to include In this PR there are a lot of changes that relate to the TAF-AD generation, but there are no changes to forward-code generation. We could add this if it does not break anything (or again a different PR, your pick). I think genmake2_hybrid fails on my MacBook, because of missing pieces for the AD code, but I didn't check closely. |
Anything related to MacBook M1/2/3 could be addressed separately. This PR seems ready to go as is, unless @antnguyen13 finds issues with running. |
| @@ -34,7 +34,7 @@ AD_TAF_FLAGS="-reverse $DIVA_FLAG -l taf_ad.log $AD_TAF_FLAGS" | |||
| FTL_TAF_FLAGS="-forward -l taf_ftl.log $FTL_TAF_FLAGS" | |||
| SVD_TAF_FLAGS="-reverse -forward -pure -l taf_svd.log $SVD_TAF_FLAGS" | |||
|
|
|||
| TAF_COMMON_FLAGS="-i4 -r4 -intrinsic system,flush" | |||
| TAF_COMMON_FLAGS="$TAF_COMMON_FLAGS -i4 -r4 -intrinsic system,flush" | |||
There was a problem hiding this comment.
This change makes it possible add TAF_COMMON_FLAGS to genmake_local, however, it duplicates the functionality of TAF_EXTRA (see, for example, verification/lab_sea/build/genmake_local). There is also a genmake2-flag -taf_extra that allows specifying extra flags for TAF. Given these available options, I do not see why we need a third way of specifying extra flags for TAF.
Further, TAF_COMMON_FLAGS was intended as an internal abbreviation for flags common to all versions of TAF/TAMC, so using it for a different purpose may be confusing in the future.
What changes does this PR introduce?
What is the current behaviour?
(You can also link to an open issue here)
What is the new behaviour
(if this is a feature change)?
Does this PR introduce a breaking change?
(What changes might users need to make in their application due to this PR?)
Other information:
Suggested addition to
tag-index(To avoid unnecessary merge conflicts, please don't update
tag-index. One of the admins will do that when merging your pull request.)