Generalized Tecplot ASCII and Binary Readers/Writers #95
Conversation
…ional data a multiple FE zone types
Codecov ReportAttention: Patch coverage is
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. |
|
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? |
|
This is ready for review. I updated the docs and bumped the minor version. |
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. |
|
@eirikurj asked to wait until he regains access to do a final review and then he can merge. |
|
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 @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") |
|
@eytanadler and I discovered that it's possible for I implemented a property in 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? |
|
I would vote for removing the redundant data |
| @@ -1154,8 +1154,8 @@ def _readZoneHeader(self, lines: List[str], iCurrent: int) -> Tuple[Dict[str, An | |||
| headerString = ", ".join(header) | |||
|
|
|||
| # Use regex to parse the header information | |||
| zoneNameMatch = re.search(r'zone t\s*=\s*"([^"]*)"', headerString, re.IGNORECASE) | |||
| zoneName = zoneNameMatch.group(1) if zoneNameMatch else None | |||
| zoneNameMatch = re.search(r'(zone t)\s*=\s*[\'""]?([^\'""\n,]+)[\'""]?(?=[,\n]|$)', headerString, re.IGNORECASE) | |||
There was a problem hiding this comment.
For future people's sake, can you add a comment here explaining what this regex matches?
There was a problem hiding this comment.
Yeah I need to review and add more tests for the regex patterns. To my knowledge, there's no established rules published by Tecplot that describe the formatting of the ASCII header data. I'm considering simpler approach that removes the regex and just assumes all data follows an = sign and terminates with either a comma or newline unless it's surrounded by single or double quotes.
There was a problem hiding this comment.
@lamkina this is looking great! I tested this locally, but overall I wanted to do a few other tests and review the unit tests more. However, I dont want to delay this so see my very minor current comments now, and I'll do another pass for the diff later.
| @@ -490,48 +499,42 @@ def writeMassesTecplot(self, filename): | |||
|
|
|||
| filename: str | |||
| filename for writing the masses. This string will have the | |||
| .dat suffix appended to it. | |||
| # .dat suffix appended to it if it does not already have it. | |||
| for i in range(len(points)): | ||
| f.write(f"{points[i][0]:f} {points[i][1]:f} {points[i][2]:f}\n") | ||
| # Build the FETriangle data array | ||
| dataArrays = np.zeros((len(self.p0) * 3, 3), dtype=float) |
There was a problem hiding this comment.
Nitpicking here, but why use this large array? Makes it more readable to just use x etc. If there is no need to keep the data together, I would favor readability. This appears in other classes as well.
| @@ -0,0 +1,1840 @@ | |||
| import re | |||
There was a problem hiding this comment.
This comment is for the documentation (no code issues here). For clarity, we should separate out the documentation of the tecplotIO module into a subsection in the docs.
| # ============================================================================== | ||
| # ASCII WRITERS | ||
| # ============================================================================== | ||
| def writeArrayToFile( |
There was a problem hiding this comment.
This routine does not have any unit tests, testing it directly. Should we add tests here, since its not a member function and could be used outside the writer?
| with self.assertRaises(TypeError, msg=msg): | ||
| TecplotOrderedZone(123, {"X": X, "Y": Y, "Z": Z}, solutionTime=0.0, strandID=-1) | ||
|
|
||
| msg = "Solution time must be a float" |
There was a problem hiding this comment.
This msg (and others below) are not used
| def writeArrayToFile( | ||
| arr: npt.NDArray[np.float64], | ||
| handle: TextIO, | ||
| maxLineWidth: int = 4000, |
There was a problem hiding this comment.
4000 used to be the limit years ago. The data format guide indicates 32000 characters for the ASCII format (see here)
Please update here and elsewhere.
|
|
||
| Parameters | ||
| ---------- | ||
| line : str |
There was a problem hiding this comment.
Description missing compared to inputs
| strandIDMatch = re.search(r"strandid\s*=\s*(\d+)", headerString, re.IGNORECASE) | ||
| strandID = int(strandIDMatch.group(1)) if strandIDMatch else -1 | ||
|
|
||
| headerDict = { |
There was a problem hiding this comment.
Just a thought (not an action item), but there are other fields available that we are not implementing for reading or writing, so maybe we should put a note somewhere that this is a limited reader/writer?
One potentially useful field is the AUXDATA.
Another one is the datatype DT (list of datatypes for the variables). However, for most of our cases, this is not that important, as we tend to write mostly floats as opposed to int.
Other variables that might be useful are the variable sharing and zone connectivity sharing (both we dont use).
| with open(filename, "r") as handle: | ||
| lines = handle.readlines() | ||
|
|
||
| pattern = r"[\s,\t\n\r]+" # Separator pattern |
There was a problem hiding this comment.
For compatibility and maintenance, it would be better to just join/combine the Enum values here instead of the string (and then adding +).
| def _validateVariables(self) -> None: | ||
| """Check that all zones have the same variables.""" | ||
| if not all(set(self.zones[0].variables) == set(zone.variables) for zone in self.zones): | ||
| raise ValueError("All zones must have the same variables.") |
There was a problem hiding this comment.
Would be great to add tests to catch these exceptions (and elsewhere)
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
.datfiles and ADflow binary surface.pltfiles.Expected time until merged
A few weeks
Type of change
Testing
Run the tests added for the Tecplot IO module.
Checklist
flake8andblackto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable