Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reader classes #1536

Merged
merged 46 commits into from Aug 19, 2021
Merged

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented Jul 21, 2021

Overview

This PR adds Reader classes that provides more fine-grained control of reading data files. Some classes include the ability to inspect and enable/disable the reading of point and cell arrays.

closes #1520

Details

Future enhancements will include inspecting and setting time points to read, and more specific details for specific readers like vtkOpenFOAMReader, for example the ability to turn on and off the cell data to point data during data read.

The provided example shows the usage of the new class.

TODO:

  • Implement all XMLReaders
  • Implement openFOAM Reader
  • Implement GenericEnsightReader
  • Add list of supported file types
  • Tests <- OpenFOAM tested locally and the ".p*" type files not tested
  • Use observer functionality similar to pyvista.read Not needed?

@MatthewFlamm MatthewFlamm marked this pull request as draft July 21, 2021 15:55
@codecov
Copy link

codecov bot commented Jul 21, 2021

Codecov Report

Merging #1536 (efde7c6) into main (fab7f77) will increase coverage by 0.01%.
The diff coverage is 68.90%.

@@            Coverage Diff             @@
##             main    #1536      +/-   ##
==========================================
+ Coverage   90.07%   90.08%   +0.01%     
==========================================
  Files          68       69       +1     
  Lines       12984    13139     +155     
==========================================
+ Hits        11695    11836     +141     
- Misses       1289     1303      +14     

@akaszynski
Copy link
Member

This is great!

@MatthewFlamm
Copy link
Contributor Author

Did some more digging on the vtkDataSetReader, which is used for '.vtk' files. vtkDataReader docs has the most useful methods. It has a much different kind of structure than the ones listed in the TODO list. Data is only differentiated by scalars vs. vector and as far as I can tell, you can only turn off/on all scalars or vectors, etc.

If the concept of scalars and vectors is added natively to pyvista as in #1431, then I'd propose we can, at that time, add this functionality into this class along with the vtkDataSetReader.

@MatthewFlamm
Copy link
Contributor Author

Are there any openFOAM datasets we can use for testing? I cannot find any. Interestingly, the file in the backwards_facing_step download is named "foam_case_0_0_0_0.case" indicating it may have been generated using openFOAM??? @akaszynski uploaded this datafile, so you may be able to shed light on this.

@adeak
Copy link
Member

adeak commented Jul 21, 2021

The other issue linked in the linked issue has a link to a repo with a foam example #1511 -> https://github.com/fbob/PyVista_OpenFOAM_Tutorial. This was originally posted in slack, then I pointed this out in #1510 (comment).

@MatthewFlamm
Copy link
Contributor Author

The other issue linked in the linked issue has a link to a repo with a foam example #1511 -> https://github.com/fbob/PyVista_OpenFOAM_Tutorial. This was originally posted in slack, then I pointed this out in #1510 (comment).

This data has no license associated with it, so I don't think we can use it? Maybe @fbob can comment on its usability to be included in pyvista/vtk-data. This would greatly help in adding openFOAM support to have a dataset to test against.

I could also cook up a quick example, but an already existing one would be preferable.

@MatthewFlamm
Copy link
Contributor Author

I was going to add in observer directly into the Reader class, but now that we have a context manager, I think it is better to let the user manage it. I've removed that from my TODO. We could consider removing it from pyvista.read as well.

with pyvista.VtkErrorCatcher() as error_catcher:
    reader = pyvista.Reader("some_file.vtp")
    mesh = reader.read()

@MatthewFlamm
Copy link
Contributor Author

This PR is good to go except for testing the OpenFOAM Reader. I have a proprietary dataset for which I tested manually and it works. I'm going to mark this as "Ready for review".

I have another branch that adds in the OpenFOAM specific functionality of selecting mesh zones/patches and turning on and off the cell-to-point data translation. We could consider dropping the OpenFOAM reader in this PR and then I can add the whole OpenFOAM Reader with all the bells and whistles later.

@MatthewFlamm MatthewFlamm marked this pull request as ready for review July 22, 2021 17:19
@MatthewFlamm MatthewFlamm changed the title WIP: Add Reader class Add Reader class Jul 22, 2021
@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Jul 26, 2021

One complication with this approach is a potential ".pvd" reader. See #414. The array information only exists in the sub-files in ".pvd" format. I was trying to keep this first PR smaller, but I wanted to raise some thornier implications downstream here.

An existing XMLReader could be nested inside to represent the active file about to be read:

reader = Reader("file.pvd")
reader.set_time_value(1.0)
print(reader.active_file_reader.cell_arrays)
reader.read()  # equivalent to reader.active_file_reader.read()

But this approach only works if there is one file per time point. If there are N files identified with different part numbers, we would have N different readers like:

reader = Reader("file.pvd")
reader.set_time_value(1.0)
print(reader.active_file_readers[0].cell_arrays)
reader.read()  # equivalent to pyvista.MultiBlock([i_reader.read() for i_reader in reader.active_file_readers])

This would be similar to how you can select and deselect data inside paraview itself. However in both of these cases, the PVDReader class would not be able to have the methods like cell_arrays defined directly, but these are defined in the Reader class in the PR as it stands. We need a different class structure than the current PR, so this doesn't cause issues later. I'll mark as Draft in the meantime.

@MatthewFlamm MatthewFlamm marked this pull request as draft July 26, 2021 20:21
@akaszynski
Copy link
Member

@MatthewFlamm, is this ready for review?

@MatthewFlamm
Copy link
Contributor Author

It is ready for review. I'm only working on getting the documentation included after #1588 was merged.

@MatthewFlamm
Copy link
Contributor Author

I will need some guidance on the documentation as I have to stumble wildly through sphinx and rst. I copied and pasted my way to get a working state for one group of Reader Classes, the XML RectilinearGrid Readers. Since all the methods and attributes are part of the classes that are inherited, BaseReader and PointCellDataSelection, there is really only an example shown . Should we include all inherited methods/attributes for the readers, using :inherited-members:, or is the link to the super classes enough?

image

@akaszynski
Copy link
Member

I will need some guidance on the documentation as I have to stumble wildly through sphinx and rst. I copied and pasted my way to get a working state for one group of Reader Classes, the XML RectilinearGrid Readers. Since all the methods and attributes are part of the classes that are inherited, BaseReader and PointCellDataSelection, there is really only an example shown . Should we include all inherited methods/attributes for the readers, using :inherited-members:, or is the link to the super classes enough?

Let me take a look at this later today and evaluate if we should include inherited or not.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reworked the documentation a bit to follow our template pattern:
tmp

I prefer having all inherited methods explicitly shown rather than telling the user to navigate through additional layers of documentation ("Flat is better than nested")

Other than the changes to the docs, I'm happy with this PR. Approved and nice work! Merge when ready.

@MatthewFlamm
Copy link
Contributor Author

This looks nice!

There are a few things that I'd like to clean up before merging: 1) Duplicate MultiBlock reference in the toctree and 2) to add the get_reader function somewhere

@MatthewFlamm
Copy link
Contributor Author

I tried doing this, but my ability to build the documentation seems to have broken again.

I get a ton of "autosummary: stub file not found" errors even when building from your last commit in this PR, which seems weird to me. As you can see, none of the pages get linked correctly here.

image

@akaszynski
Copy link
Member

akaszynski commented Aug 19, 2021

I tried doing this, but my ability to build the documentation seems to have broken again.

Sometimes I have to run make -C doc clean (or the recently added make -C doc clean-except-examples to get the docs to build correctly. Let me check this locally.

@akaszynski
Copy link
Member

Build looks great and links work. Made one final change:
sc0
sc1

@MatthewFlamm MatthewFlamm merged commit 267687f into pyvista:main Aug 19, 2021
@MatthewFlamm MatthewFlamm deleted the enh/add-reader-class branch August 19, 2021 22:55
@akaszynski akaszynski mentioned this pull request Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More flexible reader class with ability to introspect data file
3 participants