If you put it this way, I'll definitely vote for (b).
M.
On Nov 13, 2012, at 8:33 PM, Jean-Michel Campin wrote:
> Hi Martin and others,
>
> OK, I admit my explanations were not very clear.
>
> If we go with solution (b) (which I tend to prefer):
> - we can keep Real and Integer flags as $(PARMNAME)inFwdMode
> and $(PARMNAME)inAdMode (since there is always a way to initialise
> $(PARMNAME)inAdMode to an unlikely value; done in autodiff_readparms.F,
> before reading the namelist AUTODIFF_PARM01).
> - but for logical flags, we would keep also $(PARMNAME)inFwdMode but
> will need to have a $(PARMNAME)switchInAd (or switchInAd$(PARMNAME))
> which will reverse the value of $(PARMNAME) once we are in Ad-Mode.
> - the same problem does not really exist for run-time params that turns
> on/off a full pkg (e.g., useKPP) since they are read-in much earlier,
> from data.pkg, and the code we have now works with no problem. So we can
> decide to keep the logic and the names we currently have (e.g,
> useKPPinFwdMode & useKPPinAdMode).
> In term of changes, the new parameters will be:
> SEAICEuseDYNAMICSswitchInAd (in namelist AUTODIFF_PARM01)
> SEAICEuseFREEDRIFTswitchInAd (in namelist AUTODIFF_PARM01)
> SEAICEuseFREEDRIFTinFwdMode (not in any namelist)
> we will keep:
> SEAICEuseDYNAMICSinFwdMode (not in any namelist)
> and both SEAICEuseDYNAMICSinAdMode and turnFreeDriftInAdMode will be removed.
>
> If we go with solution (a), we need to move the call to AUTODIFF_READPARMS
> to the last position in the sequence of calls of packages_readparms.F
> (and assume we will never have to move it earlier); but the parameter
> names can stay as thery are now: $(PARMNAME)inFwdMode & $(PARMNAME)inAdMode
> (even for logical $(PARMNAME parameters). turnFreeDriftInAdMode would need
> to be replaced by SEAICEuseFREEDRIFTinFwdMode + SEAICEuseFREEDRIFTinAdMode
>
> Cheers,
> Jean-Michel
>
> On Fri, Nov 02, 2012 at 08:28:10AM +0100, Martin Losch wrote:
>> Hi Jean-Michel, Patrick,
>>
>> I think I would go with Jean-Michel's option (b) (provided that I understand his email correctly after reading it at least 4 times (o:),
>> and I also think that the naming convention $(PARMNAME)inFwdMode and $(PARMNAME)inAdMode is useful and intuitive and we should maintain it. (or adopt it)
>>
>> Martin
>>
>> On Oct 27, 2012, at 11:48 PM, Jean-Michel Campin wrote:
>>
>>> Hi Patrick,
>>>
>>> There was a problem of SEAICEuseDYNAMICSinAdMode being set to True
>>> even with SEAICEuseDYNAMICS=F in data.seaice,
>>> and 1 verification experiment (lab_sea/input_ad.noseaicedyn)
>>> was failing.
>>>
>>> But regarding your 1rst email:
>>>
>>> 1) I agree that the turnFreeDriftInAdMode code needs to be changed.
>>> but I would prefer to maintain our naming convention (SEAICEuseDYNAMICSinAdMode
>>> does not fit since it's not stored in SEAICE*.h header file but in
>>> AUTODIFF_PARAMS.h ! inAdModeSEAICEuseDYNAMICS name would fit better)
>>> 2) we should do something cleaner if, in packages_readparms.F,
>>> the position of CALL AUTODIFF_READPARMS within the sequence of other
>>> PKGs read_params calls matters.
>>> I see 2 options:
>>> a) move the call to AUTODIFF_READPARMS in the last position in
>>> packages_readparms.F
>>> but: (I) it is still not perfect, since there are params which are set or
>>> reset later on, until set_params.F is called.
>>> (II) might become a problem if , for some reason, 1 pkg/autodiff params
>>> has to be known early on.
>>> b) split AUTODIFF_READPARMS in 2 parts: 1rst part just to read data.autodiff
>>> and 2nd part (i,e, 2nd S/R), which can be called from set_params.F, to set
>>> the values of XXXinFwdMode and XXXinAdMode (or the equivalent of XXXinAdMode).
>>>
>>> 3) Although is not yet an issue (not too many of these switches),
>>> could be a good thing to try to separate different packages (e.g.,
>>> not to have to include all parameter header files in autodiff_readparms.F)
>>> I don't think it would be too difficult to maintain pkg separation in
>>> autodiff_inadmode_set/unset_ad.F (just need to have 1 small S/R per pkg
>>> to set/reset XXX); but regarding autodiff_readparms.F, it might be
>>> more tricky if the start in the wrong direction.
>>>
>>> 4) we need to be careful with logical parameters, because setting
>>> the default of XXXinAdMode to True (or False) before reading data.autodiff
>>> might not work if we want to allow to switch logical flag XXX in
>>> adjoint mode while keeping it's current value in Fwd mode, but not to
>>> change anything when using the default (empty namelist).
>>> With solution (a), we would do:
>>> XXXinAdMode = XXX
>>> read XXXinAdMode from data.autodiff
>>> and then just set:
>>> XXXinFwdMode = XXX
>>> this might make it more difficult to maintain pkg separation (point 3).
>>> But with solution (b), we should do:
>>> switchXXXinAdMode = FALSE
>>> read switchXXXinAdMode from data.autodiff
>>> and in 2nd S/R:
>>> XXXinFwdMode = XXX
>>> and in autodiff_inadmode_set_ad.F:
>>> IF ( switchXXXinAdMode ) XXX = .NOT.XXXinFwdMode
>>> and in autodiff_inadmode_unset_ad.F:
>>> IF ( switchXXXinAdMode ) XXX = XXXinFwdMode
>>>
>>> And finally, solution (b) can be simplified (no need for 2nd S/R)
>>> if we store directly XXXinFwdMode = XXX from ${PKG}_readparams.F
>>> (just need to include AUTODIFF_PARAMS.h there).
>>> And with solution (a), to maintain pkg separation, the storage + initialisation
>>> of XXXinAdMode and XXXinFwdMode can also be moved to ${PKG}_readparams.F
>>> (again, just need to include AUTODIFF_PARAMS.h there).
>>>
>>> I tend to prefer solution (b), but would not be a big deal if we decide to
>>> go with (a).
>>>
>>> Cheers,
>>> Jean-Michel
>>>
>>> On Sat, Oct 27, 2012 at 03:21:41PM -0400, Patrick Heimbach wrote:
>>>>
>>>> Hi Jean-Michel,
>>>>
>>>> I just saw you added following comment, and it fits the discussion thread I started yesterday:
>>>>
>>>> #ifdef ALLOW_SEAICE
>>>> 104 C- Note: should re-visit how to turn on/off pkg internal-params in adjoint mode
>>>> 105 C This will not work for PKGs which load their params AFTER this S/R is called
>>>> 106 C (currently, it is OK for SEAICE but not for all PKGs !)
>>>> 107 SEAICEuseDYNAMICSinFwdMode = SEAICEuseDYNAMICS
>>>> 108 #endif
>>>>
>>>> I'm aware of this problem, but it doesn't change the fact that we will need to have this capability (e.g., change viscosity values, potentially change advection schemes - if that works, etc). I've been trying to point out this issue back when the first discussions started on changing autodiff_inadmode_set..., addind data.autodiff, etc.
>>>> Perhaps we can try to include some checks to ensure that indeed those data.<pkg> values have been read previously.
>>>>
>>>>
>>>>
>>>> On Oct 26, 2012, at 8:50 PM, Patrick Heimbach <heimbach at mit.edu> wrote:
>>>>
>>>>>
>>>>> Hi All,
>>>>>
>>>>> in order to allow more switches in autodiff_inadmode_set_ad
>>>>> I would like to change the naming and assigning of current seaice parameter
>>>>> turnFreeDriftInAdMode somewhat:
>>>>> Naming should be SEAICEuseFREEDRIFTinAdMode
>>>>> and should be assigned like the newly introduced parameter SEAICEuseDYNAMICSinAdMode
>>>>> One reason is to be more easily searchable/grep-able in conjunction with its regular parameter.
>>>>>
>>>>> I realize that the current logic for turnFreeDriftInAdMode has the advantage of not requiring
>>>>> including SEAICE_PARAMS.h in autodiff_readparams.F, but:
>>>>> 1. it's a special case;
>>>>> 2. it's not clear what "turn" means (switch?);
>>>>> 3. sooner or later we'll need to add parameter headers anyways, e.g. when we want to temporarily change values of parameters during adjoint leg (which we already do in certain applications).
>>>>> So we might as well start early to bring some consistency into parameter naming
>>>>> (clean identification between regular parameter name and its inAdMode inFwdMode counterpart).
>>>>>
>>>>> Any objection?
>>>>>
>>>>> Cheers
>>>>> p.
>>>>>
>>>>> Begin forwarded message:
>>>>>
>>>>>> From: Patrick Heimbach <heimbach at forge.csail.mit.edu>
>>>>>> Subject: [MITgcm-cvs] MITgcm/pkg/autodiff CVS Commit
>>>>>> Date: October 26, 2012 8:38:21 PM EDT
>>>>>> To: mitgcm-cvs at mitgcm.org
>>>>>> Reply-To: MITgcm-cvs at mitgcm.org
>>>>>>
>>>>>> Update of /u/gcmpack/MITgcm/pkg/autodiff
>>>>>> In directory forge:/tmp/cvs-serv21710
>>>>>>
>>>>>> Modified Files:
>>>>>> AUTODIFF_PARAMS.h autodiff_inadmode_set_ad.F
>>>>>> autodiff_inadmode_unset_ad.F autodiff_readparms.F
>>>>>> Log Message:
>>>>>> Add inAdMode switch for SEAICEuseDYNAMICS
>>>>>> (extend logic in ADAUTODIFF_INADMODE_SET)
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> MITgcm-cvs mailing list
>>>>>> MITgcm-cvs at mitgcm.org
>>>>>> http://mitgcm.org/mailman/listinfo/mitgcm-cvs
>>>>>
>>>>>
>>>>> ---
>>>>> Patrick Heimbach | heimbach at mit.edu | http://www.mit.edu/~heimbach
>>>>> MIT | EAPS 54-1420 | 77 Massachusetts Ave | Cambridge MA 02139 USA
>>>>> FON +1-617-253-5259 | FAX +1-617-253-4464 | SKYPE patrick.heimbach
>>>>>
>>>>> _______________________________________________
>>>>> MITgcm-devel mailing list
>>>>> MITgcm-devel at mitgcm.org
>>>>> http://mitgcm.org/mailman/listinfo/mitgcm-devel
>>>>
>>>>
>>>> ---
>>>> Patrick Heimbach | heimbach at mit.edu | http://www.mit.edu/~heimbach
>>>> MIT | EAPS 54-1420 | 77 Massachusetts Ave | Cambridge MA 02139 USA
>>>> FON +1-617-253-5259 | FAX +1-617-253-4464 | SKYPE patrick.heimbach
>>>>
>>>
>>>
>>>
>>>> _______________________________________________
>>>> MITgcm-devel mailing list
>>>> MITgcm-devel at mitgcm.org
>>>> http://mitgcm.org/mailman/listinfo/mitgcm-devel
>>>
>>>
>>> _______________________________________________
>>> MITgcm-devel mailing list
>>> MITgcm-devel at mitgcm.org
>>> http://mitgcm.org/mailman/listinfo/mitgcm-devel
>>
>>
>> _______________________________________________
>> MITgcm-devel mailing list
>> MITgcm-devel at mitgcm.org
>> http://mitgcm.org/mailman/listinfo/mitgcm-devel
>
> _______________________________________________
> MITgcm-devel mailing list
> MITgcm-devel at mitgcm.org
> http://mitgcm.org/mailman/listinfo/mitgcm-devel