New default value for DerivedQuantities.show_units
#892
Conversation
RemDelaporteMathurin
changed the title
DerivedQuantities.show_units
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #892 +/- ##
==========================================
- Coverage 99.56% 99.24% -0.32%
==========================================
Files 61 61
Lines 2750 2796 +46
==========================================
+ Hits 2738 2775 +37
- Misses 12 21 +9 ☔ View full report in Codecov by Sentry. |
| @property | ||
| def export_unit(self): | ||
| if self.field == "T": | ||
| return "K" | ||
| else: | ||
| return "H m-3" | ||
|
|
There was a problem hiding this comment.
If you're going to add this new property everywhere, it should also go in the doc strings
| @@ -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.
Could maybe add this new default to the doc string
There was a problem hiding this comment.
Well if it's not an argument then it's not really a default value. Like we don't document the fact that data has a "default" value of [] in the docstrings
| mesh = f.UnitIntervalMesh(10) | ||
| V = f.FunctionSpace(mesh, "P", 1) |
There was a problem hiding this comment.
could import the mesh and function space from .tools.py? not necessary though
test/unit/test_exports/test_derived_quantities/test_derived_quantities.py
Outdated
Show resolved
Hide resolved
| 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.
Actually, seen as only the mobile solute can used in the flux calculation, is it worth hard coding this?
| quantity_title = f"{self.field} flux surface {self.surface}" | |
| quantity_title = f"solute flux surface {self.surface}" |
There was a problem hiding this comment.
Well I wouldn't cause we may move away from "solute" at some point
| 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.
Again see previous comment on surface flux. Might be mistaken though
There was a problem hiding this comment.
see previous comment
|
Also might be worth adding another test for all the derived quantities without units, as many aren't being test at the moment (avg_surf, avg_vol, max_surf, max_vol, min_surf, min_vol, point_value, tot_surf, tot_vol). Just to get the full coverage |
Co-authored-by: James Dark <[email protected]>
Fixes #860 but instead of deprecating the
show_unitsargument, I just have it default to True.What do you think @jhdark @KulaginVladimir? I thought it'd be nice to keep the functionality in case someone doesn't want the units in (eg. they are working in moles)...