-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add display precision to log_diagnostics_plugin
#6118
Changes from all commits
3fabb59
6a8908f
02bbb33
c903bb0
d018ff2
7042f31
f3028c6
f6e0f0b
14d4415
ea4e567
5ff64fb
b25209e
22a4fb7
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 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -28,6 +28,32 @@ pub struct DiagnosticMeasurement { | |||||||||||||||
pub value: f64, | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// The number of digits that should be displayed for a diagnostic. | ||||||||||||||||
/// | ||||||||||||||||
/// Choose `IntegerValue` for discrete values and `DecimalValue(num_of_decimals)` to specify the number of decimals. | ||||||||||||||||
#[derive(Debug)] | ||||||||||||||||
pub enum DisplayPrecision { | ||||||||||||||||
IntegerValue, | ||||||||||||||||
DecimalValue(usize), | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
impl DisplayPrecision { | ||||||||||||||||
pub fn num_decimals(&self) -> usize { | ||||||||||||||||
match self { | ||||||||||||||||
DisplayPrecision::IntegerValue => 0, | ||||||||||||||||
DisplayPrecision::DecimalValue(x) => *x, | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// trait DiagnosticDisplayPercision { | ||||||||||||||||
// type DisplayPercision; | ||||||||||||||||
// } | ||||||||||||||||
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. Please do not leave intentionally commented out code in the codebase. If it's not meant to be included, please remove it. Otherwise, please complete the implementation. 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. I left it in in regards to this:
But if there is no answer I'll remove it :) |
||||||||||||||||
|
||||||||||||||||
// impl DiagnosticDisplayPercision for Diagnostic { | ||||||||||||||||
// type DisplayPercision = DisplayPercision; | ||||||||||||||||
// } | ||||||||||||||||
|
||||||||||||||||
Comment on lines
+49
to
+56
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.
Suggested change
|
||||||||||||||||
/// A timeline of [`DiagnosticMeasurement`]s of a specific type. | ||||||||||||||||
/// Diagnostic examples: frames per second, CPU usage, network latency | ||||||||||||||||
#[derive(Debug)] | ||||||||||||||||
|
@@ -41,6 +67,7 @@ pub struct Diagnostic { | |||||||||||||||
ema_smoothing_factor: f64, | ||||||||||||||||
max_history_length: usize, | ||||||||||||||||
pub is_enabled: bool, | ||||||||||||||||
pub precision: DisplayPrecision, | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
impl Diagnostic { | ||||||||||||||||
|
@@ -73,11 +100,12 @@ impl Diagnostic { | |||||||||||||||
.push_back(DiagnosticMeasurement { time, value }); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// Create a new diagnostic with the given ID, name and maximum history. | ||||||||||||||||
/// Create a new diagnostic with the given ID, name, maximum history and decimal precision. | ||||||||||||||||
pub fn new( | ||||||||||||||||
id: DiagnosticId, | ||||||||||||||||
name: impl Into<Cow<'static, str>>, | ||||||||||||||||
max_history_length: usize, | ||||||||||||||||
precision: DisplayPrecision, // Choose number of decimals for display precision. | ||||||||||||||||
) -> Diagnostic { | ||||||||||||||||
let name = name.into(); | ||||||||||||||||
if name.chars().count() > MAX_DIAGNOSTIC_NAME_WIDTH { | ||||||||||||||||
|
@@ -98,6 +126,7 @@ impl Diagnostic { | |||||||||||||||
ema: 0.0, | ||||||||||||||||
ema_smoothing_factor: 2.0 / 21.0, | ||||||||||||||||
is_enabled: true, | ||||||||||||||||
precision, | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
@@ -195,6 +224,11 @@ impl Diagnostic { | |||||||||||||||
pub fn clear_history(&mut self) { | ||||||||||||||||
self.history.clear(); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// Get the number of decimals to display for this diagnostic. | ||||||||||||||||
pub fn num_decimals(&self) -> usize { | ||||||||||||||||
self.precision.num_decimals() | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// A collection of [Diagnostic]s | ||||||||||||||||
|
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.
Why have an enum for this?
Running the code in the rust playground:
This prints:
I don't think an enum is necessary? You can just use
0
to print an integer.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 just added the enum. Just using a usize is how I did it to begin with.
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.
Hmm. @alice-i-cecile I have a hard time understanding why turning this into an enum is better. Can you clarify why?
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.
Certain diagnostics will always be integer-valued: users should not be able to configure the number of decimals for them. Other diagnostics are float-like, and users may want to round to the nearest whole number.
To me, these cases are distinct (even though the same number of decimal places are shown). Representing that in the type system means that we'll be able to expose the correct controls to users later.
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.
Do
IntegerValue
andDecimalValue(0)
have different behavior?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.
Not in the current version of this PR. I'll play with this by next Monday, and try to get a full design here as a PR to @zeroacez branch :)