Use Readers in fileio module

[MatthewFlamm]

Overview

Closes #2494. This PR uses the machinery in the pyvista.utilities.reader module to replace the functionality that the standard_reader_routine did.

This enables one location to maintain reading utilities.

Details

I will leave read_exodus out of this PR as there is no Reader class and it is self-contained in fileio.

TODO:

• implement read_plot3d functionality into the Reader
• Add error observing functionality into BaseReader? Continue wrapping observer for pyvista.read
• Eliminate standard_reader_routine

[codecov]

Codecov Report

Merging #2561 (e4c8fcf) into main (b6ab604) will increase coverage by 0.03%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             main    #2561      +/-   ##
==========================================
+ Coverage   93.72%   93.76%   +0.03%     
==========================================
  Files          75       75              
  Lines       16116    16150      +34     
==========================================
+ Hits        15105    15143      +38     
+ Misses       1011     1007       -4     

[MatthewFlamm]

@whophil I see you implemented the read_plot_3d function. I'd like to discuss this with you. I reimplemented the q-file functionality into the Reader class, but have run into some issues. One is that we have yet another method in vtk 9.1.0 for adding q-files, SetFileNames. The tests mock out the call to the vtk object, so it wasn't caught.

My reading of https://vtk.org/doc/nightly/html/classvtkPlot3DMetaReader.html suggests that it is expected that the user put this information into a more complex json formatted file before reading it. There are many more possibilities other than q-files here. Unless we want to implement a wrapper that mimics the json format, I'd suggest we scrap the q-file functionality entirely. Thoughts?

For example, vtk suggests that this is a more descriptive pattern to use for this format, note that the changes in q-file are linked to a time point, an xyz file, and a function file.

{ "time" : 3.5, "xyz" : "combxyz.bin", "q" : "combq.1.bin", "function" : "combf.1.bin" },
{ "time" : 4.5, "xyz" : "combxyz.bin", "q" : "combq.2.bin", "function" : "combf.2.bin" }

[whophil]

Hey @MatthewFlamm , sorry for the delay. I haven't been keeping up with the changes you've been making to the readers, so excuse my ignorance here.

• After your refactoring, will read_plot3d() still exist?
• If so, are you proposing to remove its q_filenames argument?

I have never used the vtkPlot3DMetaReader. My understanding is that the JSON format it reads is either an invention of ParaView or VTK, and I have not worked with any code that outputs it. It seems just to store the flags that are used by the actual reader (vtkMultiBlockPLOT3DReader). My most common use case is to just use read_plot3d() to read in a single grid file and a single q file.

[MatthewFlamm]

• After your refactoring, will read_plot3d() still exist?

Im not sure, that's what I wanted to discuss. I'm proposing that all the other routines other than pyvista.read will be deprecated and/or removed. It will be suggested to use the class based readers for cases where more control is needed, or where inspection of the reader state is needed, like:

reader = Plot3DMetaReader(filename)
reader.add_q_files(q_file)
mesh = reader.read()

After looking into this, it isn't clear to me that adding q-files is intended to be user-facing. It has a different method name in 8.x, 9.0.x, and 9.1.x. I'm unsure how we can get the functionality to more closely resemble the json file format without a lot of effort, which from my reading of the vtk docs is the intended usage for this reader. Maybe the best solution is to replicate the q file usage in the class based Reader, then someone with more need can make it more elegant?

[whophil]

I believe the thing that should be wrapped, and possibly exposed with a convenience class (like the CGNS reader) is actually vtkMultiBlockPLOT3DReader and not vtkPlot3DMetaReader. Regardless, the ability to read Plot3d data with Q files is in my opinion a necessity. I'm happy to help with the Plot3D refactor, as I likely use Plot3D file formats more than you.

[MatthewFlamm]

believe the thing that should be wrapped, and possibly exposed with a convenience class (like the CGNS reader) is actually vtkMultiBlockPLOT3DReader and not vtkPlot3DMetaReader

You have uncovered something I overlooked. pyvista.read uses vtkPlot3DMetaReader while read_plot_3d uses vtkMultiBlockPLOT3DReader. This clears up a lot of my confusion. I think we can add a new reader for vtkMultiBlockPLOT3DReader with the q file functionality.

[MatthewFlamm]

This is now fully ready for review.

I fully removed read_legacy, but we could consider deprecating it instead.

There aren't tests for all the file types, so it would be nice if someone can review the mapping of readers based on extension in the old fileio module with the mapping in reader module to make sure I didn't make any typos/mistakes on the extensions (or missing like .img was missing).

[adeak]

I can only look at the meat of the changes later, so only a few cursory remarks for now.

[adeak]

Is the situation of read_legacy() any different from read_exodus()? If I understand correctly the legacy reader is what reads .vtk and .pvtk files. I'm just wondering if downstream code might rely on this, and whether still having read_exodus() is justified then.

Most of the hits at https://github.com/search?l=Python&q=read_legacy&type=Code are from something called openWB, and most of the rest are from pyvista itself (including forks). (Which is to say they aren't downstream uses of our helper.)

[MatthewFlamm]

We should either deprecate or remove read_legacy IMO. Even before this PR when you called pyvista.read('file.vtk') it called read_legacy, so there was no need to have a separate function.

Exodus currently does not have a Reader class and it is self contained in that it doesn't use standard_reader_routine. So I specifically left it out if this PR to keep things manageable.

[whophil]

Two thoughts:

1. Eventually MultiBlockPlot3DReader should have properties for all of the useful reader flags. The ParaView property window for vtkMultiBlockPlot3DReader is shown below. Not having these does not block this PR, in my opinion.
   

2. test_reader::test_multiblockplot3dreader reports the following to stdout. The data reads fine, but the presence of the error might mislead users.

   2022-05-12 00:06:15.803 (   0.525s) [          BF9E13]vtkMultiBlockPLOT3DRead:2555   ERR| vtkMultiBlockPLOT3DReader (0x7fab5381c6f0): The settings that you provided do not match what was auto-detected in the file. Set `ForceRead` to true to get past this error.
   The detected settings are: 
   BinaryFile: 1
   ByteOrder: 1
   HasByteCount: 1
   MultiGrid: 1
   NumberOfDimensions: 3
   DoublePrecision: 1
   IBlanking: 0
   

   This error is issued because vtkMultiBlockPlot3DReader appears to "test" the settings of the reader against the auto-detected settings when UpdateInformation() is called, but only if auto-detect is False.

   One solution would be to set auto-detect to True by default, and to rework BaseReader so that attribute can be correctly set before the call to UpdateInformation(), which is triggered by the BaseReader.__directory setter. Alternately, MultiBlockPlot3DReader.__init__ could just override BaseReader.__init__, but that seems less maintainable.

[MatthewFlamm]

1. Eventually MultiBlockPlot3DReader should have properties for all of the useful reader flags.

I agree. This PR should make it much easier to add those in the future. This PR adds the existing functionality.

test_reader::test_multiblockplot3dreader reports the following to stdout.

Good catch! I refactored the __init__ of BaseReader so that we no longer need to override the whole __init__ method to set defaults. I used this in this reader and the other Readers.

[akaszynski]

This is a way cleaner implementation, and I fully support moving to use our new get_reader approach.

I see no issues with this and I'd like to get this merged to main and start playing around with this locally on some of my closed source projects to make sure that the new reader works. I'm happy with this as-is.

[MatthewFlamm]

This is a fairly large PR. I'll give it just a few more days in case there are any more comments. I agree that testing downstream will also be important as there wasn't comprehensive testing of file types in fileio.

[akaszynski]

I'll give it just a few more days in case there are any more comments.

Agreed. I'll merge by Friday next week unless there are additional concerns raised.