[petsc-dev] perhaps where the custom adapativity perversity began

[Jed Brown]

Barry Smith <bsmith at mcs.anl.gov> writes:

>> On Apr 29, 2017, at 10:49 PM, Jed Brown <jed at jedbrown.org> wrote:
>> 
>> Barry Smith <bsmith at mcs.anl.gov> writes:
>> 
>>>    ierr = PetscOptionsBool("-ts_theta_adapt","Use time-step adaptivity with the Theta method","",th->adapt,&th->adapt,NULL);CHKERRQ(ierr);
>> 
>> https://bitbucket.org/petsc/petsc/commits/3b1890cda3a41f3c81730c41d1c1f55cb4d78b93
>> 
>>  Added time-step adaptivity for the theta method. It's currently
>>  switched off by default till the error estimation is fixed.
>> 
>> And this is why workarounds are so much worse than fixing the root of the problem.
>
>  But why does theta need its own flag for controlling adaptivity when TS also has a flag; new types are usually/always made by copying old implementations and then modifying them, thus bad code gets propagated to new methods. Why could this workaround just use the general adapt instead of adding its own option?

The error estimation requires storage of extra vectors and the
implementation was broken.  For whatever reason, Shri wanted TSTheta to
use TSADAPTNONE by default and probably thought it would be better to
have an explicit option than to overwrite it.  I don't see the point in
litigating what was running through his head at the time.  But we see
similar patterns frequently, where something doesn't really work and
there are some hacks to make up for it.  The problem is that those hacks
aren't removed even if the fundamental problem is fixed.

I recall a couple months back you wanted to accept some hacky code in a
PR (dealing with adjoints and HBM) with intent to fix it eventually, but
that was exactly the sort of thing you would freak out about if you
encountered it later.  The usual way to combat this is to refuse to
merge until the temporary workarounds are fixed.  That makes it harder
to get code merged (bad), but helps avoid the kludgy code.

>
>
>
>> 
>>>   Really, even a method so simple minded I can understand it, requires its own flags for adaptivity?
>>> 
>>>   Based on this model we'll have -ksp_cg_max_its and -ksp_gmres_max_its pretty soon and where is -ts_arkimex_max_steps, we'd better add it soon.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20170429/741d97dd/attachment.sig>