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.

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

Fps permutations test #12335

Merged
merged 14 commits into from
Jul 15, 2024
Merged

Fps permutations test #12335

merged 14 commits into from
Jul 15, 2024

Conversation

AviaAv
Copy link
Contributor

@AviaAv AviaAv commented Oct 29, 2023

Tracked on [LRS-909]

@AviaAv AviaAv requested a review from Nir-Az October 29, 2023 15:07
@Nir-Az Nir-Az requested review from maloel and removed request for maloel October 29, 2023 18:42
@Nir-Az
Copy link
Collaborator

Nir-Az commented Oct 29, 2023

@AviaAv always check if your test indeed run in all required OS.
Hint: you are missing some definition on which device to run..

@Nir-Az Nir-Az requested a review from OhadMeir October 29, 2023 19:06
@AviaAv AviaAv requested review from Nir-Az and OhadMeir November 2, 2023 11:19
@Nir-Az
Copy link
Collaborator

Nir-Az commented Nov 5, 2023

Please rebase development and run CI manually with nightly context and verify the test run and pass on all supported OS's

f"{sensor_profile_dict[sensor_key][0].stream_name()} is {expected_fps_dict[sensor_key]}"
f" on {get_resolution(sensor_profile_dict[sensor_key][0])}\n")
s = s.replace("on (0, 0)", "") # remove no resolution for Motion Module profiles
s += "***************"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove. Might be helpful for debug but we don't want this in the final logs

funcs_dict = generate_functions(sensor_fps_dict)

for key in sensor_profiles_dict:
sensor = key
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use for sensor in ...

sensor_fps_dict[key] /= TIME_TO_COUNT_FRAMES
# now for each sensor we have the average fps received

sensor = key
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

#############################################
# ---------- Core Test Functions ---------- #
#############################################
def check_fps(expected_fps, fps_measured):
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename check_fps_dict. Also it is better to have measured fps as the first argument for consistency with our other check functions use (and also your own check_fps_pair).

# to avoid having a long run time, we don't choose the same stream more than once
if p.stream_type() not in stream_types_added:
if p.stream_type() == rs.stream.infrared:
if p.stream_index() != 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Style comment (decide if you want to change) - this is very indented you can use and instead of the last if, or better yet, move the last 3 ifs to a function should_add_profile

if p.stream_type() not in stream_types_added:
if p.stream_type() == rs.stream.infrared:
if p.stream_index() != 1:
continue # on some devices, using Infrared 1 seems to have better fps than IR/IR2
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not clear - which models and why...
According to our talk with Idan you can use validation KPIs are for depth+IR1 not IR2

@Nir-Az Nir-Az closed this Dec 7, 2023
@Nir-Az Nir-Az reopened this Dec 7, 2023
@AviaAv AviaAv force-pushed the fps-perm-test branch 2 times, most recently from e159742 to e35a008 Compare March 12, 2024 07:15
@Nir-Az Nir-Az closed this Jul 8, 2024
@Nir-Az Nir-Az reopened this Jul 8, 2024
@Nir-Az Nir-Az merged commit 9194348 into IntelRealSense:development Jul 15, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants