[petsc-dev] PETSC_VIEWER_*_() vs PetscOptionsGetViewer()

[Smith, Barry F.]

  MatViewFromOptions(A,NULL,"-mat_view");
  VecViewFromOptions(v,NULL,"-vec_view");

$ PETSC_OPTIONS="" ./ex1000 -mat_view :testfile -vec_view :testfile::append
$ more testfile 
Mat Object: 1 MPI processes
  type: seqaij
row 0: (0, 0.0718163)  (2, 0.720032) 
row 1: (0, 0.217676)  (1, 0.397778) 
row 2: (0, 0.354647)  (2, 0.984052) 
Vec Object: 1 MPI processes
  type: seq
0.
0.
0.


  VecViewFromOptions(v,NULL,"-vec_view");
  MatViewFromOptions(A,NULL,"-mat_view");

$ PETSC_OPTIONS="" ./ex1000 -mat_view :testfile -vec_view :testfile::append
$ more testfile 
Mat Object: 1 MPI processes
  type: seqaij
row 0: (0, 0.0718163)  (2, 0.720032) 
row 1: (0, 0.217676)  (1, 0.397778) 
row 2: (0, 0.354647)  (2, 0.984052) 

  I'm not opposed to fixing up the current situation I just submit that we don't know how to fix up the current situation



> On May 28, 2019, at 8:02 PM, Matthew Knepley <knepley at gmail.com> wrote:
> 
> On Tue, May 28, 2019 at 5:46 PM Smith, Barry F. <bsmith at mcs.anl.gov> wrote:
> 
> 
> > On May 28, 2019, at 4:40 PM, Matthew Knepley <knepley at gmail.com> wrote:
> > 
> > On Tue, May 28, 2019 at 5:26 PM Smith, Barry F. via petsc-dev <petsc-dev at mcs.anl.gov> wrote:
> > 
> >   I have concerns. 
> > 
> >   Without the use of singletons 
> > 
> >    -mat_view binary -vec_view binary
> > 
> >   will result in only the final object viewed being in the file binaryoutput? The idea behind the singletons is they provide continuity for the entire run. 
> > 
> >   Note that currently when also creating and destroying the viewers with the same name you only get the most recent stuff. In fact something like 
> > 
> >    -mat_view hdf5:myfile 
> > 
> >   is likely now buggy since it will only save the most recent matrix viewed in the file the user might expect all the matrices in there.
> > 
> > you use
> > 
> >   0mat_view hdf5:myfile::append
> 
>    And all the crap from yesterday's runs are also in the file?
> 
> Barry,
> 
>   -mat_view binary -vec_view binary:::append
> 
>     Matt
>  
> > 
> > 
> >    Matt
> >  
> > Perhaps PetscOptionsGetViewer() needs to register all viewers created with the destroy in PetscFinalize() and never have the user destroy them and attach them to the communicator so they get reused at the next call instead of creating a new one.
> > 
> >   Even if we do the above another problem is
> > 
> >    -xxx_view xx:filename 
> > 
> > will be problematic if there are xxx in different communicators since if we attach them to a communicator we won't find them with a different communicator and will open the file again. I don't know how to fix this.
> > 
> >   I think the entire PetscOptionsGetViewer() business needs a bit more thought. At the moment it is a bit flawed. Making them non-singletons won't fix the flaws.
> > 
> > > BTW all PETSC_VIEWER_XXX_() manpages say "Creates a XXX PetscViewer shared by all processors in a communicator."
> > > This is apparently not true - they are singletons stashed to the communicator by MPI_Comm_set_attr() and next time reused if found using MPI_Comm_get_attr(), right?
> > > So it should be Returns, not Creates.
> > 
> >   This should be fixed in its own PR.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > > On May 28, 2019, at 10:35 AM, Hapla Vaclav via petsc-dev <petsc-dev at mcs.anl.gov> wrote:
> > > 
> > > Let me follow up discussion on creating/controlling viewers from options.
> > > 
> > > Barry agrees to rename PetscOptionsGetViewer() to PetscViewerCreateFromOptions() in Issue #291.
> > > 
> > > But now I can see a problem: it mixes Get and Create behavior, as it returns the singleton provided by PETSC_VIEWER_*_() if possible, otherwise creates a new viewer using PetscViewerCreate(). So actually both names PetscOptionsGetViewer() and PetscViewerCreateFromOptions() are confusing.
> > > 
> > > I think this design is unnecessarily complicated and not really useful.
> > > I don't see any advantage of reusing the singleton in this context.
> > > And leads to some IMHO unexpected behavior:
> > > * -myviewer hdf5:: and -myviewer hdf5 is not the same! I didn't know that but hdf5:: creates a new viewer and hdf5 reuses the singleton.
> > > * Any properties (including options prefix) set to the viewer returned by PetscOptionsGetViewer() actually affect the singleton and vice versa.
> > >   (See https://bitbucket.org/petsc/petsc/commits/324f959)
> > > 
> > > So in my opinion PetscOptionsGetViewer()
> > > * should really be renamed PetscViewerCreateFromOptions(),
> > > * should _always_ return a new instance and call PetscViewerSetFromOptions() on it,
> > > * could also set the passed prefix to the viewer, i.e. PetscViewerCreateFromOptions(comm, options, "pre_", ...) would result in the viewer having prefix pre_ 
> > > 
> > > Do you think it would break anything?
> > > 
> > > Thanks,
> > > Vaclav
> > > 
> > > BTW all PETSC_VIEWER_XXX_() manpages say "Creates a XXX PetscViewer shared by all processors in a communicator."
> > > This is apparently not true - they are singletons stashed to the communicator by MPI_Comm_set_attr() and next time reused if found using MPI_Comm_get_attr(), right?
> > > So it should be Returns, not Creates.
> > 
> > 
> > 
> > -- 
> > What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> > -- Norbert Wiener
> > 
> > https://www.cse.buffalo.edu/~knepley/
> 
> 
> 
> -- 
> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> -- Norbert Wiener
> 
> https://www.cse.buffalo.edu/~knepley/