-
Notifications
You must be signed in to change notification settings - Fork 25
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
PointValue class #655
PointValue class #655
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 56 57 +1
Lines 2113 2124 +11
=======================================
+ Hits 2090 2101 +11
Misses 23 23 ☔ View full report in Codecov by Sentry. |
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.
Hi @KulaginVladimir thanks for this! This is a first round of comment.
class TestCompute: | ||
mesh = f.UnitIntervalMesh(10) | ||
V = f.FunctionSpace(mesh, "P", 1) | ||
|
||
c = f.interpolate(f.Expression("x[0]", degree=1), V) | ||
|
||
x = 1 | ||
my_value = PointValue("solute", x) | ||
my_value.function = c | ||
|
||
def test_minimum(self): | ||
expected = self.c(self.x) | ||
|
||
produced = self.my_value.compute() | ||
assert produced == expected |
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.
Could we also test try to test this on a multi-dimensional mesh?
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.
@RemDelaporteMathurin you mean 2D/3D?
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.
Yes
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.
Can it me done as:
@pytest.mark.parametrize(
"mesh,x", [(f.UnitIntervalMesh(10), 1), (f.UnitCubeMesh(10, 10, 10), (1, 0, 1))]
)
def test_point_compute(mesh, x):
V = f.FunctionSpace(mesh, "P", 1)
c = f.interpolate(f.Expression("x[0]", degree=1), V)
my_value = PointValue("solute", x)
my_value.function = c
expected = c(x)
produced = my_value.compute()
assert produced == expected
or the TestCompute
class should remain?
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.
I believe this you work just fine!
def test_title_H(): | ||
x = 1 | ||
field = "solute" | ||
my_value = PointValue(field, x) | ||
assert my_value.title == "{} value at [{}]".format(field, x) | ||
|
||
|
||
def test_title_T(): | ||
x = 1 | ||
field = "T" | ||
my_value = PointValue(field, x) | ||
assert my_value.title == "{} value at [{}]".format(field, x) |
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 can merge these two tests using pytest parametrized tests.
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.
Shall we modify all other tests (e.g. test_total_volume
, test_total_surface
, etc.) in the same way?
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.
Ideally yes but let's do this in a separate dedicated PR.
Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>
Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>
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.
Thanks for this @KulaginVladimir!
I'll leave the PR open a while longer if anyone has other comments.
Proposed changes
This PR introduces the PointValue class according to #569. It will allow users to export the
field
value at a specific point during simulations.Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
Note
Please note, I've tried to implement the class directly based on #569, but real simulation failed to run due to "TypeError: c-0 argument after * must be an iterable, not int". Thus, I've additionally ensured that x = [x] for 1D simulations. Possible, @RemDelaporteMathurin and @jhdark will provide a better solution.