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

Smith, Barry F. bsmith at mcs.anl.gov
Wed May 29 17:44:55 CDT 2019 

   I am not advocating that you use the singletons. I agree that you using a filename etc is better for many situations especially for HDF. So this discussion is not really about singletons.

   My point is that the PetscViewerGetFromOptions() could register each opened viewer somewhere and search the current list to reuse one that has already been opened (ignoring for now the issue of viewers opening on different communicators). They can register the destruction of the viewer with PetscObjectRegisterDestroy() so the destruction happens when the program ends. Thus it truly is a Get() and one is not suppose to destroy them. Once one gets created it is available until the program ends. No need to worry about append being in the right place.

  If one is paranoid about losing data because a viewer wasn't closed earlier in the program we could add a close option (goes where the append 
option goes)  that forces the viewer to be closed upon a call to PetscViewerGetFromOptions() but it could keep a list of all the files it has opened and if the same viewer is requested again after this call it will be automatically reopened with append so as not to lose the earlier data. But since this is reopening the file saved earlier from the same run we don't have the issue of append that would include data from previous runs when one does not want that.

  Concerns?

   Barry

  I think this resolves all the issues except of the same file/socket/draw being obtained for a different communicator than the one it was originally created for. Which may not be resolvable.







> On May 29, 2019, at 6:11 AM, Hapla Vaclav <vaclav.hapla at erdw.ethz.ch> wrote:
> 
> 
> 
>> On 29 May 2019, at 01:00, Smith, Barry F. <bsmith at mcs.anl.gov> wrote:
>> 
>> 
>> 
>>> On May 28, 2019, at 5:36 PM, Václav Hapla <vaclav.hapla at erdw.ethz.ch> wrote:
>>> 
>>> 
>>> 
>>> 28. května 2019 23:26:23 SELČ, "Smith, Barry F." <bsmith at mcs.anl.gov> napsal:
>>>> 
>>>> 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. 
>>> 
>>> OK. Then I would for now suggest reusing the singleton only for binary. For hdf5 and probably others it has no obvious advantage.
>> 
>>  What about the socket viewer. Or graphics viewers? They can't be open and shut repeatedly. 
>> 
>>   I think even for HDF5 etc the constant open and close model is wrong (and requiring that crazy append option that keeps whatever crazy stuff that was in the file yesterday is wrong).
> 
> It's true that even for HDF5 it's better to avoid multiple opens and closes (not as critical as for some other viewers, though). But if I call PetscViewerCreateFromOptions() and it is guaranteed to create a new viewer, I know I must take responsibility and call PetscViewerDestroy() only once I have written everything. It is an informed decision. If I need this context to span different scopes, I would use e.g. PetscObjectCompose() or make my own global variable to propagate this viewer rather than relying on the singletons, which I have not that much confidence in, honestly, because they do too much in background to take such informed decisions. My general opinion is that in case of the file output, one should choose the file name, mode, format and various type-specific properties explicitly (and at least name and mode without any defaults), and should know exactly when the file is opened and when it is closed - because it can all be very expensive in time and storage space and you should know exactly what you are doing.
> 
> However, I can't exclude that I'm too much HDF5-centric and you know more about all other viewers :-)
> But they obviously shouldn't be mapped to communicators (as you mentioned on your own) but to the destinations - I don't mean the argument of PETSC_VIEWER_*_() but where the underlying singleton is stored.
> 
> So if you insist on the PETSC_VIEWER_*_() model, I think there should some sort of global table of viewer types and destinations, and something like PetscViewerFindFromOptions() which would find or create the corresponding viewer when needed. This would be called from all that PETSC_VIEWER_*_(), and also usable directly.
> 
> Vaclav
> 
>> 
>>   On the other hand I'm not convinced there shouldn't be a way to close a viewer from this process earlier then in PETSc finalize but I don't have a good model for handling this.
>> 
>>> 
>>> In outlook, what about adding some kind of offset table to the info files so that we would e.g. able to jump directly to the second object without first reading the first. This table could even include names, so that we would be able to unify binary and hdf5 behavior in this respect. With a bit of care we wouldn't loose backward compatibility with older binary files, they would be the same but info files would have additional information.
>>> 
>>>> 
>>>> 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.
>>>> 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.
>>> 
>>> Not sure if I got it right, but this is also a problem of the current implementation since e.g. PETSC_VIEWER_BINARY_WORLD and PETSC_VIEWER_BINARY_SELF are stored independently, and can be both returned by PetscOptionsGetViewer(), right? I can test this situation.
>> 
>>  Oh, yes it is definitely a problem with current implementation, both singletons and non-singletons. If you find a solution to this I think I'll let you do anything you want :-)
>> 
>> 
>>> 
>>> Vaclav
>>> 
>>>> 
>>>> 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.

More information about the petsc-dev mailing list