[petsc-dev] TS failures in master

[Barry Smith]

> On Apr 29, 2017, at 4:05 PM, Lisandro Dalcin <dalcinl at gmail.com> wrote:
> 
> On 29 April 2017 at 21:26, Barry Smith <bsmith at mcs.anl.gov> wrote:
>> 
>>  Lisandro,
>> 
>>   Even though it may be "irrelevant" it could be/is confusing to users to sometimes have something printed and sometimes not. They may conclude that there is a bug or misunderstand the output due to this by concluding there probably is some bound, it is just not being printed.
>> 
> 

  I am having trouble finding the branch/source code we are talking about. Could someone please tell me that?


> Barry, in my PR #699, the only subtype that will print nothing is
> TSADAPTNONE. I wrote things so licence noise for that particular
> subtype which does, well, nothing... All the other controllers use and
> print the dt_min/dt_max values.
> 

   It looks like the current proposed code is?

+    if (adapt->safety < 1) {ierr = PetscViewerASCIIPrintf(viewer,"  safety factor %g\n",(double)adapt->safety);CHKERRQ(ierr);}
+    if (adapt->reject_safety < 1) {ierr = PetscViewerASCIIPrintf(viewer,"  extra safety factor after step rejection %g\n",(double)adapt->reject_safety);CHKERRQ(ierr);}
+    if (adapt->clip[1] < PETSC_MAX_REAL) {ierr = PetscViewerASCIIPrintf(viewer,"  clip fastest increase %g\n",(double)adapt->clip[1]);CHKERRQ(ierr);}
+    if (adapt->clip[0] > 0) {ierr = PetscViewerASCIIPrintf(viewer,"  clip fastest decrease %g\n",(double)adapt->clip[0]);CHKERRQ(ierr);}
+    if (adapt->dt_max < PETSC_MAX_REAL) {ierr = PetscViewerASCIIPrintf(viewer,"  maximum allowed timestep %g\n",(double)adapt->dt_max);CHKERRQ(ierr);}
+    if (adapt->dt_min > 0) {ierr = PetscViewerASCIIPrintf(viewer,"  minimum allowed timestep %g\n",(double)adapt->dt_min);CHKERRQ(ierr);}



   If it is the base TSAdaptView() then I think the decision about printing dt_min and dt_max should not be based on the value 
of dt_min dt_max but should be based on if dt_min and max are actually used by the method so not 

if (dt_min > 0) print

 but rather

if (not None type adapt) print 

 
If I understand correctly with the current if () if I used basic or glee and manually set dt_min to zero the view of basic or glee would not show the value of 0. This I do not want, nor do I think you want it, I definitely want it to show the 0 value, because it means something for basic or glee and if nothing is printed I will be confused.

I'm pretty sure the right model is to remove almost all those prints from the TSAdaptView() (since they don't apply to NONE) and put them in TSAdaptView_Basic() and TS_AdaptView_Glee(). To avoid code duplication you could put them into a utility function that gets called in both places.

Am I totally missing something here? But to recap when you say 

> the only subtype that will print nothing is
> TSADAPTNONE. I wrote things so licence noise for that particular
> subtype which does, well, nothing... All the other controllers use and
> print the dt_min/dt_max values.

I think this is completely wrong. Because the if () are based on the numerical values if the user sets the numerical values to the extreme the values will not print for basic and glee and I think they should always be printed for basic and glee even if 0 etc.





> Now my PR got outdated, I'been waiting for
> 
>>   An alternative to not printing any thing when 0 is to print a different message indicating no clipping, for example something like
>> 
>>>> if (adapt->dt_min > 0) {
>>>>  ierr = PetscViewerASCIIPrintf(viewer,"  minimum allowed timestep %g\n",(double)adapt->dt_min);CHKERRQ(ierr);
>>>> } else {
>>      ierr = PetscViewerASCIIPrintf(viewer,"  no minimum on the allowed timestep\n");CHKERRQ(ierr);
>> }
>> 
> 
> What kind of useful information would that provide with you run with
> -ts_adapt_type none ? And as I said above, this is the only case where
> the min/max dt (as well as safety factors,etc.) is silenced.
> 
> 
>>   So if you really don't want to have it print the 0 or the MAX please use this construct for clarity. Is that reasonable?
>> 
> 
> Now my PR got outdated, I'been waiting for Toby to merge another, but
> this has not happened yet.
> 
> @Satish Once I merge PR 699 I'll revert some of your changes to the
> View() method. Then we can discuss the whole thing again.
> 
> -- 
> Lisandro Dalcin
> ============
> Research Scientist
> Computer, Electrical and Mathematical Sciences & Engineering (CEMSE)
> Extreme Computing Research Center (ECRC)
> King Abdullah University of Science and Technology (KAUST)
> http://ecrc.kaust.edu.sa/
> 
> 4700 King Abdullah University of Science and Technology
> al-Khawarizmi Bldg (Bldg 1), Office # 0109
> Thuwal 23955-6900, Kingdom of Saudi Arabia
> http://www.kaust.edu.sa
> 
> Office Phone: +966 12 808-0459