refactor: response time class api #313
Conversation
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
rokamu623
changed the title
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
| t_in = range_records.get_column_series(input_column) | ||
| t_out = range_records.get_column_series(output_column) |
There was a problem hiding this comment.
Although it is also used in the function _to_timeseries(), it's better to change t_in/out to timestamps_in/out or somethings more comprehensible.
There was a problem hiding this comment.
This PR will not be changed in this PR as it is written in accordance with existing code.
| def to_best_case_records(self) -> RecordsInterface: | ||
| records = self._records.to_range_records() | ||
| input_column = records.columns[1] | ||
| output_column = records.columns[-1] | ||
| return self._to_records(input_column, output_column) | ||
|
|
||
| def to_worst_case_records(self) -> RecordsInterface: | ||
| records = self._records.to_range_records() | ||
| input_column = records.columns[0] | ||
| output_column = records.columns[-1] | ||
| return self._to_records(input_column, output_column) |
There was a problem hiding this comment.
Although it is also used in the function from line 743, I think that using to_range_records is not best way just for column name.
Although there is no way to get column names in ResponseTime class, it is better that, for example, create a new variable self._input_records = records in the ResponseTime class and then write as input_column = f'{self._input_records.columns[0]}_min or max'.
However, it is a little more complicated to write than before.
There was a problem hiding this comment.
This PR will not be changed in this PR as it is written in accordance with existing code.
| {'start': 6, 'end': 6}, | ||
| ] | ||
| result = to_dict(response.to_records(all_pattern=True)) | ||
| result = to_dict(response.to_best_case_stacked_bar()) | ||
| assert result == expect_raw |
There was a problem hiding this comment.
Is it intentional that 'start' : 4 is not taken into account?
There was a problem hiding this comment.
The cross_case may be different from the desired specification. (The implement may wrong.)
However, as this PR is a refactoring, this investigation and correction will not be conducted.
| result = to_dict(response.to_response_records()) | ||
| assert result == expect_raw | ||
|
|
||
| expect_raw = [ | ||
| {'start': 0, 'end': 3}, | ||
| {'start': 0, 'end': 5}, | ||
| {'start': 2, 'end': 3}, | ||
| ] | ||
|
|
||
| result = to_dict(response.to_records(all_pattern=True)) | ||
| result = to_dict(response.to_stacked_bar()) | ||
| assert result == expect_raw |
There was a problem hiding this comment.
In this test case, it seems not to consider the case: 'end' : 5 in line 238.
Is this OK?
There was a problem hiding this comment.
The cross_case may be different from the desired specification. (The implement may wrong.)
However, as this PR is a refactoring, this investigation and correction will not be conducted.
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
|
@nabetetsu |
Signed-off-by: rokamu623 <[email protected]>
There was a problem hiding this comment.
rt2-886_result.zip
LGTM 👍
I have confirmed that there is no difference between the visualization results of Stacked bar and Timeseries before and after this PR's modification.
Description
Implement
to_best_case_records()andto_worst_case_records()in theResponseTime class, which are APIs in the same format asto_records()inLatencyclass.Related links
Jira: https://tier4.atlassian.net/browse/RT2-886
Notes for reviewers
Pre-review checklist for the PR author
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.