Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add sim time to plot APIs (only non-supported APIs) #433

Merged
merged 36 commits into from
Dec 11, 2023
Merged

feat: add sim time to plot APIs (only non-supported APIs) #433

merged 36 commits into from
Dec 11, 2023

Conversation

xygyo77
Copy link
Contributor

@xygyo77 xygyo77 commented Nov 20, 2023 •
edited
Loading

Description

Modify the following API to support sim_time.

  • create_response_time_stacked_bar_plot()
  • create_response_time_timeseries_plot()
  • create_response_time_histogram_plot()
  • create_latency_histogram_plot()
  • create_period_histogram_plot()
  • create_frequency_histogram_plot()

Related links

https://tier4.atlassian.net/browse/RT2-1083

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.

  • The PR has been properly tested.
  • The PR has been reviewed.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • (Optional) The PR has been properly tested with CARET_report verification.
  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

xygyo77 and others added 9 commits November 14, 2023 22:54
@xygyo77
create_response_time_timeseries_plot() API for sim_time.
97fcbc4 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
Add support sim_time to histogram APIs.
dc28432 
Signed-off-by: ISP akm <[email protected]>
@emori-ctc @xygyo77
fix: unified graph captions. (tier4#410)
79ef91b 
* Unified graph captions.

Signed-off-by: emb4 <[email protected]>

* Unified conditional branching of graph captions.

Signed-off-by: emb4 <[email protected]>

* Fixed flake8 errors.

Signed-off-by: emb4 <[email protected]>

* Point corrected.

Signed-off-by: emb4 <[email protected]>

* Corrected string notation.

Signed-off-by: emb4 <[email protected]>

* Corrected string notation.

Signed-off-by: emb4 <[email protected]>

* Corrected the points pointed out.

Signed-off-by: emb4 <[email protected]>

* Corrected caption.

Signed-off-by: emb4 <[email protected]>

* Corrected caption.

Signed-off-by: emb4 <[email protected]>

---------

Signed-off-by: emb4 <[email protected]>
@iwatake2222 @xygyo77
chore(histogram): display number for Y-axis instead of probability (t…
57cb877 
…ier4#430)

* chore(histogram): display number instead of probability

Signed-off-by: takeshi.iwanari <[email protected]>

* fix: change hover label

Signed-off-by: takeshi.iwanari <[email protected]>

---------

Signed-off-by: takeshi.iwanari <[email protected]>
@xygyo77
Add sim_time support to create_response_time_stacked_bar_plot() API
8ac5704 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
Histogram legends click policy is set to "hide"
49a8ab9 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
Merge remote-tracking branch 'upstream/main' into feat/add-sim-time-t…
1669962 
…o-plot-api
@xygyo77
Skip invalid records that are involved in sim_time conversion but are…
ecf7c40 
… invalid.

Signed-off-by: ISP akm <[email protected]>
@pre-commit-ci
ci(pre-commit): autofix
a2448b6
@xygyo77 xygyo77 requested a review from isp-uetsuki November 20, 2023 03:16
@xygyo77 xygyo77 changed the title Feat: add sim time to plot APIs (only non-supported APIs) feat: add sim time to plot APIs (only non-supported APIs) Nov 20, 2023
xygyo77 and others added 14 commits November 20, 2023 12:27
@xygyo77
chore: spell miss coverter->converter
246070b 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
Merge remote-tracking branch 'origin/feat/add-sim-time-to-plot-api' i…
7eb1111 
…nto feat/add-sim-time-to-plot-api
@xygyo77
fixed pytest errors
4339386 
Signed-off-by: ISP akm <[email protected]>
@pre-commit-ci
ci(pre-commit): autofix
f178713
@xygyo77
fixed pytest errors
a543617 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
fixed pytest errors
88b20fd 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
fixed pytest errors
b010cd2 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
fixed pytest errors
6ab9083 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
fixed pytest errors
9896f0f 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
fixed pytest errors
bda755c 
Signed-off-by: ISP akm <[email protected]>
@pre-commit-ci
ci(pre-commit): autofix
1b299df
@xygyo77
fixed pytest errors
0a2f1e0 
Signed-off-by: ISP akm <[email protected]>
@xygyo77
Merge remote-tracking branch 'origin/feat/add-sim-time-to-plot-api' i…
fe8264a 
…nto feat/add-sim-time-to-plot-api
@xygyo77
fixed pytest errors
1e5d3ef 
Signed-off-by: ISP akm <[email protected]>
isp-uetsuki
isp-uetsuki requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@isp-uetsuki isp-uetsuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I suggest creating utility function to get ClockConverter from object (Communication, Callback, etc.) to recude redundancy. My suggestions are:
    • adding util.py to caret_analyze/plot
    • adding get_clock_converter function to util.py above
  • I suggest adding simulation time support to StackedBarPlot.to_dataframe. (I think it does not work currently because LatencyStackedBar.to_dataframe does not support simulation time.)
  • I suggest removing MetricsBase._convert_timeseries_records_to_sim_time and using conversion functionality of metrics objects (Frequency, ResponseTime, etc.) instead to reduce redundancy.

src/caret_analyze/plot/metrics_base.py Outdated Show resolved Hide resolved
src/caret_analyze/plot/visualize_lib/bokeh/bokeh.py Outdated Show resolved Hide resolved
src/caret_analyze/plot/visualize_lib/bokeh/bokeh.py Outdated Show resolved Hide resolved
src/caret_analyze/plot/visualize_lib/bokeh/stacked_bar.py Outdated Show resolved Hide resolved
src/caret_analyze/plot/visualize_lib/bokeh/stacked_bar.py Outdated Show resolved Hide resolved
isp-uetsuki
isp-uetsuki requested changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@isp-uetsuki isp-uetsuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think the meaning of is_base_ts_convert argument of Frequency.to_records is a little bit difficult to understand. I suggest to remove this argument by:
    • removing converter argument from FrequencyTimeSeries._get_timestamp_range. Now, min_time and max_time in FrequencyTimeSeries.to_timeseries_records_list are always in system time.
    • removing is_base_ts_convert argument of Frequency.to_records and Frequency._get_frequency_with_timestamp.

isp-uetsuki
isp-uetsuki approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@isp-uetsuki isp-uetsuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

miyakoshi-dev
miyakoshi-dev approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@miyakoshi-dev miyakoshi-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xygyo77 xygyo77 merged commit 840a9db into tier4:main Dec 11, 2023
10 checks passed
isp-uetsuki added a commit to isp-uetsuki/CARET_analyze that referenced this pull request Dec 27, 2023
@isp-uetsuki
Revert "feat: add sim time to plot APIs (only non-supported APIs) (ti…
6473630 
…er4#433)"

This reverts commit 840a9db.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miyakoshi-dev miyakoshi-dev miyakoshi-dev approved these changes

@isp-uetsuki isp-uetsuki isp-uetsuki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xygyo77 @isp-uetsuki @miyakoshi-dev @emori-ctc @iwatake2222