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

[Barry Smith]

> On Apr 29, 2017, at 11:08 PM, Jed Brown <jed at jedbrown.org> wrote:
> 
> 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.

   Yes, TS has suffered from a lack of regular refactoring, that is really what I am complaining about, not the individual bits of ugliness.

> 
> 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.