Migrate plotting utilities away from Mayavi/TVTK to VTK#328
Conversation
Merge develop into main for new version release
V2.4 breakes the LinearAeroelastic tests, since the solver is not imported. This pull request restores the working environment for linear simulations
Fixed import issue for linear assembler
…and the plotting utilities in Modal. Included a few other small changes
…Small fix to beamplot where some beam elements were ordered incorrectly.
|
The linear test which was previously failing was because the aerogridplot post processor was changing the wake array data in place for the solution. I have left Mayavi as a dependency as there are a few little bits of legacy code which use it, but it is no longer used in the key routines (beam plot, aero grid plot and plotting modes). Should now be ready to merge - and we should also merge into main as I don't believe main works at the moment due to TVTK depreciations. |
Ben Preston (ben-l-p)
requested review from
isabella586,
kccwing and
Hui Ling Wong (wong-hl)
Hui Ling Wong (wong-hl)
left a comment
Hui Ling Wong (wong-hl)
left a comment
There was a problem hiding this comment.
Mostly minor stylistic comments. I have faith that the math and everything else is correct
|
|
||
| def discretise_domain(self): | ||
| """ | ||
| r""" |
There was a problem hiding this comment.
Have the docs just been broken for all these equations?
There was a problem hiding this comment.
I make a few of the docstrings in the code to raw strings because I was getting annoyed by warnings saying there is an invalid escape sequence (because of https://stackoverflow.com/questions/52335970/how-to-fix-syntaxwarning-invalid-escape-sequence-in-python when maths symbols are used, for example \omega). Now, if this breaks the docs on git, I need to check...
There was a problem hiding this comment.
I usually add two backslashes, i.e. \\, when I have the maths so I don't need to make it a raw string. I thought not having the backslashes meant that it just wouldn't build and display the maths properly
There was a problem hiding this comment.
Either option works for viewing the maths in my IDE, and using raw strings means the maths is the same as what it would be in Latex., and functions like this don't appear in the GitHub docs anyway. This would be a different story if it was a string that actually needed escape sequence characters. I think having either approach in this case shouldn't do us any harm.
| zeta_star = np.moveaxis(self.data.aero.timestep_info[self.ts].zeta_star[i_surf], 0, -1).copy() # [m*+1, n+1, 3] | ||
| gamma = self.data.aero.timestep_info[self.ts].gamma_star[i_surf].copy() # [m*, n] |
There was a problem hiding this comment.
This is me nitpicking but the comment of m*+1 is not really clear what it is. The +1 is what's throwing me off. And should the one comment in the line below be m* or just m
There was a problem hiding this comment.
Will change this, thanks for spotting. In my own codes, I usually denote the shape of arrays with respect to other variables in such comments, but I get why it doesn't make much sense here!
There was a problem hiding this comment.
The shape is really useful ngl. It's not trivial to visualise it from the code so the comments are welcome. I just don't understand what this one means 😅
There was a problem hiding this comment.
For your reference, I'm here using the standard SHARPy discretisation notation, which is M panels in the chord wise direction, N in the span wise direction, and a wake of length M* panels. I should probably use m_star rather than m* as that can look like multiplication. As we're working with vertex, we have to add one to the number of panels. I'll add this to the comments!
There was a problem hiding this comment.
Oh! It's m_star I thought it was a multiplication. It makes sense now.
4 participants
Also includes merging main to develop as this should have been done earlier. Some tests are failing with this branch due to changes in the linearised aerodynamic system which are under investigation.