Generalized Tecplot ASCII and Binary Readers/Writers

[lamkina]

Purpose

This PR implements generalized Tecplot readers and writers for ASCII and Binary files. I created two new data structures to hold Ordered and Finite Element Tecplot zone types. The data structures contain a dictionary for the variable data arrays, the strand ID, solution time, and for FE zones the connectivity.

There is a single read function that determines the ASCII or Binary functionality automatically based on the file extension. The same pattern is used for the writer. The structure of the data is determined automatically by the dimensions of the numpy arrays for ordered zones. For example, an array of shape (10, 10, 10) will be treated as an I=10, J=10, K=10 Tecplot ordered zone. For FE zones, the nodes are a flattened array and the connectivity should match the FE zone type (LineSeg, Quadrilateral, Triangle, etc.). The writer uses the shape of the connectivity to determine the FE zone type to write.

I added tests that confirm the reader and writer work properly, as well as tests using ADflow ASCII slice .dat files and ADflow binary surface .plt files.

Expected time until merged

A few weeks

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Run the tests added for the Tecplot IO module.

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[codecov]

Codecov Report

Attention: Patch coverage is 90.86687% with 59 lines in your changes missing coverage. Please review.

Project coverage is 51.87%. Comparing base (30aebba) to head (99e04c7).

Files with missing lines	Patch %	Lines
baseclasses/problems/pyWeight_problem.py	12.12%	29 Missing ⚠️
baseclasses/utils/tecplotIO.py	96.47%	21 Missing ⚠️
baseclasses/solvers/pyAero_solver.py	18.18%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #95       +/-   ##
===========================================
+ Coverage   41.25%   51.87%   +10.61%     
===========================================
  Files          26       27        +1     
  Lines        2596     3181      +585     
===========================================
+ Hits         1071     1650      +579     
- Misses       1525     1531        +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lamkina]

Still need to update the docs, but I wanted to get the review started. We might want to consider a minor version bump as well?

[lamkina]

This is ready for review. I updated the docs and bumped the minor version.

[gawng]

Yea, this looks good. I agree with the minor version bump. Thanks for alphabetizing the imports too

[eirikurj]

Looking very good! I was planning on doing a more thorough review, but to avoid having this sitting for long, I think there are few things there to get immediate feedback on. Please check my comments based in this first pass.

[lamkina]

@eirikurj @gawng @A-CGray

I realized we are writing Tecplot files in AeroSolver here so I will go through and check the rest of the files to make sure I replace instances of this with the new reader/writer module.

I'll re-request reviews after I finish these changes.

[lamkina]

@A-CGray @gawng @eirikurj this should be ready for another round of review

[lamkina]

@gawng @A-CGray @eirikurj bump

[lamkina]

I stress tested your implementation a bit by increasing the shape of the 3D data to (100, 100, 100) and then trying to open the files produced by the unit tests in Tecplot which resulted in some errors for the ascii format files. These failures aren't caught by the tests presumably because your file readers are less picky than tecplot's. I don't think we can get around that without having tecplot installed on the test runners so don't worry about that. However, before we merge it might be good for you to run the test suite without deleting the temporary files and verify that you can open them all in Tecplot. Also might be worth increase the amount of data you're reading/writing to stress the code more. Even with the (100,100,100) case the tests ran in 12s on my machine.

Was able to open the binary files OK though, below is an example output file from the ordered zone test:

Hmm interesting. The size of the data shouldn't have this effect. I'll recreate this stress test and add it as a unit test. As for keeping the temporary files, I would prefer to create a script that can generate the test data to do one-off type checks with Tecplot. I agree that adding Tecplot to the test runners is not a great solution.

[lamkina]

@A-CGray the insane grid structure you were seeing in Tecplot was actually correct. I was using random coordinates to generate the nodal data. I am updating to using linearly increasing nodal data so the output in Tecplot will resemble a line, square, or cube.

See here in the tests where this was happening.

[A-CGray]

@A-CGray the insane grid structure you were seeing in Tecplot was actually correct. I was using random coordinates to generate the nodal data. I am updating to using linearly increasing nodal data so the output in Tecplot will resemble a line, square, or cube.

See here in the tests where this was happening.

Oh yeah this is fine, I didn't mean to suggest anything was wrong by posting that image, was just showing evidence that I'd tested opening the binary files written by the tests in Tecplot.

[A-CGray]

Great work @lamkina !

[gawng]

I am satisfied with this. WIll be taking advantage of this code in DCFoil. Thanks!

[A-CGray]

I guess we can't merge until @eirikurj regains access and marks his requested changes as completed. Or @anilyil can override

[lamkina]

@eirikurj asked to wait until he regains access to do a final review and then he can merge.

[eytanadler]

I've now used this and can absolutely vouch for its usefulness! @lamkina and I just chatted about plotting in matplotlib. One problem is that by default matplotlib's tricontourf function includes any regions outside the CFD domain in the triangulation, so they end up getting colored by the contour. To prevent this, the tecplot file's connectivity can be passed to the tricontourf function. However, the function only accepts triangular elements, not quadrilateral. Thus, we think it'd be best to add the following method to the TecplotFEZone class to provide triangular connectivity even for quad zones.

@property
def triConnectivity(self) -> npt.NDArray:
    if self.zoneType == ZoneType.FETRIANGLE:
        return self.connectivity
    elif self.zoneType == ZoneType.FEQUADRILATERAL:
        return np.row_stack((self.connectivity[:, [0, 1, 2]], self.connectivity[:, [2, 3, 0]]))
    else:
        raise TypeError(f"triConnectivity not supported for {self.zoneType.name} zone type")

[A-CGray]

LGTM

[lamkina]

@eytanadler and I discovered that it's possible for FE Zones to have the full surface mesh nodal data and then a conn that indexes that global array. This makes things a bit clunky to access just the nodal data for a single zone. For example, you would need to do zone.data["CoordinateX"][zone.connectivity], however, this would duplicate nodes several times.

I implemented a property in TecplotFEZone called uniqueNodalData that returns only the data at unique nodes. However, this contains a dictionary with unordered nodal data variables.

Another solution might be to update the nodal data and connectivity when we read the zone to make sure we only store the minimum amount of information necessary for a given zone.

@eirikurj , @gawng , @eirikurj , and @eytanadler what are your preferences here?

[A-CGray]

I would vote for removing the redundant data

[eytanadler]

After a bit more discussion and @A-CGray's comment, @lamkina and I think defaulting to the data/connectivity unique to each zone should be the default. The original data and connectivity from the file should be kept just in case in dataRaw and connectivityRaw attributes.