-
Notifications
You must be signed in to change notification settings - Fork 24
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
New default value for DerivedQuantities.show_units
#892
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ def __init__(self, field) -> None: | |
self.T = None | ||
self.data = [] | ||
self.t = [] | ||
self.show_units = False | ||
self.show_units = True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could maybe add this new default to the doc string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well if it's not an argument then it's not really a default value. Like we don't document the fact that |
||
|
||
@property | ||
def allowed_meshes(self): | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -53,15 +53,13 @@ def export_unit(self): | |||||
|
||||||
@property | ||||||
def title(self): | ||||||
quantity_title = f"Flux surface {self.surface}: {self.field}" | ||||||
if self.show_units: | ||||||
if self.field == "T": | ||||||
quantity_title = f"Heat flux surface {self.surface}" | ||||||
else: | ||||||
quantity_title = f"{self.field} flux surface {self.surface}" | ||||||
if self.field == "T": | ||||||
quantity_title = f"Heat flux surface {self.surface}" | ||||||
else: | ||||||
quantity_title = f"{self.field} flux surface {self.surface}" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, seen as only the mobile solute can used in the flux calculation, is it worth hard coding this?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I wouldn't cause we may move away from "solute" at some point |
||||||
|
||||||
if self.show_units: | ||||||
return quantity_title + f" ({self.export_unit})" | ||||||
|
||||||
else: | ||||||
return quantity_title | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,13 +47,21 @@ class TestMakeHeader: | |
sph_surface_flux_2 = SurfaceFluxSpherical("T", 6) | ||
ads_h = AdsorbedHydrogen(1) | ||
|
||
mesh = f.UnitIntervalMesh(10) | ||
V = f.FunctionSpace(mesh, "P", 1) | ||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could import the mesh and function space from |
||
|
||
def test_simple(self): | ||
""" | ||
Tests that a proper header is made for the output .csv file | ||
when there is only one festim.DerivedQuantity object | ||
""" | ||
my_derv_quant = DerivedQuantities([self.surface_flux_1]) | ||
|
||
for quantity in my_derv_quant: | ||
quantity.function = f.Function(self.V) | ||
RemDelaporteMathurin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
header = my_derv_quant.make_header() | ||
|
||
expected_header = ["t(s)", self.surface_flux_1.title] | ||
assert header == expected_header | ||
|
||
|
@@ -68,6 +76,9 @@ def test_two_quantities(self): | |
self.tot_surf_1, | ||
] | ||
) | ||
for quantity in my_derv_quant: | ||
quantity.function = f.Function(self.V) | ||
|
||
header = my_derv_quant.make_header() | ||
expected_header = ["t(s)", self.surface_flux_1.title, self.tot_surf_1.title] | ||
assert header == expected_header | ||
|
@@ -89,6 +100,11 @@ def test_all_quantities(self): | |
self.ads_h, | ||
] | ||
) | ||
|
||
# to compute the units of the quantities they need a function | ||
for quantity in my_derv_quant: | ||
quantity.function = f.Function(self.V) | ||
|
||
header = my_derv_quant.make_header() | ||
expected_header = ["t(s)"] + [ | ||
self.surface_flux_1.title, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,12 @@ def test_title(field, surface): | |
""" | ||
|
||
my_h_flux = SurfaceFlux(field, surface) | ||
assert my_h_flux.title == "Flux surface {}: {}".format(surface, field) | ||
my_h_flux.function = c_1D | ||
if field == "T": | ||
expected_title = f"Heat flux surface {surface} ({my_h_flux.export_unit})" | ||
else: | ||
expected_title = f"{field} flux surface {surface} ({my_h_flux.export_unit})" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again see previous comment on surface flux. Might be mistaken though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see previous comment |
||
assert my_h_flux.title == expected_title | ||
|
||
|
||
class TestCompute: | ||
|
@@ -324,6 +329,7 @@ def test_cylindrical_flux_title_no_units_solute(): | |
festim.CylindricalSurfaceFlux with a solute field without units""" | ||
|
||
my_h_flux = SurfaceFluxCylindrical("solute", 2) | ||
my_h_flux.show_units = False | ||
assert my_h_flux.title == "solute flux surface 2" | ||
|
||
|
||
|
@@ -332,6 +338,7 @@ def test_cylindrical_flux_title_no_units_temperature(): | |
festim.CylindricalSurfaceFlux with a T field without units""" | ||
|
||
my_heat_flux = SurfaceFluxCylindrical("T", 4) | ||
my_heat_flux.show_units = False | ||
assert my_heat_flux.title == "Heat flux surface 4" | ||
|
||
|
||
|
@@ -357,6 +364,7 @@ def test_spherical_flux_title_no_units_solute(): | |
festim.SphericalSurfaceFlux with a solute field without units""" | ||
|
||
my_h_flux = SurfaceFluxSpherical("solute", 3) | ||
my_h_flux.show_units = False | ||
assert my_h_flux.title == "solute flux surface 3" | ||
|
||
|
||
|
@@ -365,6 +373,7 @@ def test_spherical_flux_title_no_units_temperature(): | |
festim.CSphericalSurfaceFlux with a T field without units""" | ||
|
||
my_heat_flux = SurfaceFluxSpherical("T", 5) | ||
my_heat_flux.show_units = False | ||
assert my_heat_flux.title == "Heat flux surface 5" | ||
|
||
|
||
|
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.
If you're going to add this new property everywhere, it should also go in the doc strings