-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fully parse .trm files #169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
+ Coverage 69.00% 70.27% +1.26%
==========================================
Files 11 11
Lines 713 730 +17
==========================================
+ Hits 492 513 +21
+ Misses 221 217 -4 ☔ View full report in Codecov by Sentry. |
@wtbarnes, please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I let this completely fall off my radar. I'm fine with this as is though I'm a little concerned about how brittle this code is. By that I mean it relies on the correspondence between indices and particular property names and as currently implemented, that correspondence is hardcoded twice: once when reading from the files and then again when assigning parts of the data array to each property. I don't have any bright ideas on how to deal with this though.
I'll have a think about the "brittleness" aspect of it, so don't merge it yet. It's true that hard coding the indices is not great, particularly if the |
|
Right, all indices have been removed. This isn't perfectly robust to changes in HYDRAD, but all that would need to be done is to add any new items to the lists of terms if there's a change in any of the equations. This probably isn't the most efficient since it reads the files multiple times, though. |
This might need a new unit test. I just pushed a fix that would not have given the correct result. Perhaps checking that one of the columns is not zero everywhere? |
@wtbarnes This one is ready for review. The changes should be more robust. |
Great! This looks good to me. The lack of repetition between the correspondence of the indices and variable names should make this more robust to changes in the layout. Thanks for all of your effort on this! |
Addresses #168
I've added a full parsing of the .trm files. Hopefully the naming scheme is reasonable, but happy to edit that. I've also changed the variable names for the conduction variables in the .phy files, which are more accurately heat fluxes.