-
Notifications
You must be signed in to change notification settings - Fork 186
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
Fix PiecewisePolytropicFluid Python test, add some bindings #4682
Conversation
@@ -1,19 +1,27 @@ | |||
# Distributed under the MIT License. | |||
# See LICENSE.txt for details. | |||
|
|||
import spectre.PointwiseFunctions.Hydro.EquationsOfState as spectre_eos | |||
|
|||
import unittest |
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.
@nilsvu are the imports required to be alphabetized in .py
files, or is that just with #include
in .cpp
/.hpp
files? If not, then everything looks good to me.
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.
We haven't established a rule for sorting imports yet. I started to use isort recently, which puts system modules before third-party modules, then alphabetizes.
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.
The ctest summary does not produce the long piecewise polytrope name anymore and isort has certain ordering for .py
imports, so I approve.
#include "PointwiseFunctions/Hydro/EquationsOfState/Python/PiecewisePolytropicFluid.hpp" | ||
#include "PointwiseFunctions/Hydro/EquationsOfState/Python/PolytropicFluid.hpp" |
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.
[Commit message]
This isn't a PiecewisePolynomial
it's a PiecewisePolytrope
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.
🤦♂️ fixed
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.
Looks good, just @knelli2's comment about the commit message. Your first commit still says "PiecewisePolynomial"
We can do this now that we have Tensor bindings.
cf39bc9
to
67d490f
Compare
Proposed changes
Fixes #4523. I also added some bindings while I was touching the files.
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments