-
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
Derived Quantites refresh: Units in title #736
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
+ Coverage 99.49% 99.51% +0.02%
==========================================
Files 58 58
Lines 2358 2468 +110
==========================================
+ Hits 2346 2456 +110
Misses 12 12 ☔ 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.
I think it could be worth having a units
property as it would simplify a lot of the logic.
For instance, this:
@property
def title(self):
quantity_title = f"Flux surface {self.surface}: {self.field}"
if self.show_units:
# obtain domain dimension
dim = self.function.function_space().mesh().topology().dim()
# new title but only with show_units to not introduce breaking change
quantity_title = f"{self.field} flux surface {self.surface}"
if self.field == "T":
quantity_title = f"Heat flux surface {self.surface}"
if dim == 1:
return quantity_title + " (W m-2)"
if dim == 2:
return quantity_title + " (W m-1)"
if dim == 3:
return quantity_title + " (W)"
else:
if dim == 1:
return quantity_title + " (H m-2 s-1)"
if dim == 2:
return quantity_title + " (H m-1 s-1)"
if dim == 3:
return quantity_title + " (H s-1)"
else:
return quantity_title
would be simplified to:
@property
def title(self):
quantity_title = f"Flux surface {self.surface}: {self.field}"
if self.show_units:
# new title but only with show_units to not introduce breaking change
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}"
return quantity_title + f" ({self.units})"
else:
return quantity_title
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.
lgtm, @SamueleMeschini @KulaginVladimir @gabriele-ferrero feel free to comment if you have suggestions
@@ -3,9 +3,40 @@ | |||
|
|||
|
|||
class AverageVolume(VolumeQuantity): | |||
""" | |||
Computes the average value of a field in a given volume | |||
int(f ds) / int (1 * ds) |
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.
Volume integration?
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.
Yeah, good catch! I thought I had gone through and fixed all these
@@ -4,9 +4,40 @@ | |||
|
|||
|
|||
class MaximumVolume(VolumeQuantity): | |||
""" | |||
Computes the maximum value in a field in a given volume |
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.
Computes the maximum value of a field in a given volume?
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 same for MinimumVolume
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.
Yeah that is arguably better, I'll update it
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.
Just to make it more compact
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
@jhdark are we good to merge? |
I think so |
Proposed changes
New PR restoring #711
With this change, users can opt to have units in the titles of the derived quantities file. They can do this with a new argument within the
DerivedQuantities
class like so:The unit is then added to the derived quantity's title, accounting for the mesh's dimension. For example, should a user call for a
TotalVolume
of thesolute
field in a 1D mesh case with units, the title will beTotal solute volume 1 (H m-2)
, whereas in the same case but in 2D, the title will beTotal solute volume 1 (H m-1)
.The original format of the titles is kept the same to avoid introducing breaking changes; however, it could be debated whether to change them to make them more consistent.
I have also taken the liberty to add doc strings to all derived quantities.
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...