Skip to content

Added benchmark#39

Open
LevDenisov wants to merge 21 commits intoprime-slam:stablefrom
LevDenisov:benchmark-artifact
Open

Added benchmark#39
LevDenisov wants to merge 21 commits intoprime-slam:stablefrom
LevDenisov:benchmark-artifact

Conversation

@LevDenisov
Copy link
Collaborator

@LevDenisov LevDenisov commented Aug 11, 2023

Implementation of the benchmark for C++ and Python versions of the code.

Measurements are carried out in the following groups:

  • Image reading, point cloud translation, image segmentation

  • For each stage of image segmentation

@achains achains self-requested a review August 11, 2023 15:42
@achains achains self-requested a review August 14, 2023 07:43

deplex::utils::savePointCloudCSV(test_duration.cast<float>().transpose(), (data_dir / "process_cloud.csv").string());

long long elapsed_time_mean = std::accumulate(test_duration.begin(), test_duration.end(), 0) / NUMBER_OF_RUNS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you store mean in integer (long long) type? Shouldn't it be float/double instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, because this method returns data with the type long long

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the right hand you have not only method, but an expression with division operator. In C++ terms since both std::accumulate (in your case) and number of runs return integer type you will get an integer. But we know, that mean result won't be integer for sure. Cast either result of accumulate or number of runs to double so we can get floating point division.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

end_time = std::chrono::high_resolution_clock::now();

time = std::chrono::duration_cast<std::chrono::microseconds>(end_time - start_time).count();
test_duration[i] = (int)time;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use C-style type casting. In C++ it's better to use static_cast(..)

std::filesystem::path config_path = data_dir / "config/TUM_fr3_long_val.ini";

auto start_time = std::chrono::high_resolution_clock::now();
auto end_time = std::chrono::high_resolution_clock::now();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strange formatting... Do you use clang-format?

Copy link
Collaborator Author

@LevDenisov LevDenisov Aug 14, 2023

Choose a reason for hiding this comment

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

fixed

double standard_error = standard_deviation / sqrt(NUMBER_OF_RUNS);

// 95% confidence interval
double lower_bound = (double)elapsed_time_mean - 1.96 * (standard_error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the meaning of 1.96? Instead of using hard-coded constants it's better to create a variable with clear name and than use the variable :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Put it in .gitignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need two different formats for C++ and Python? I understand that C++ code works only with the version without commas, but you can also parse same file from Python

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need orb trajectory in your code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted

def graphic():
data = np.genfromtxt(Path(data_dir) / Path('process_sequence_50_snapshot.csv'), delimiter=',')

# Создаем boxplot
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mix of russian and english

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


plt.xticks([0], ['stable'])

plt.ylabel("Время (мс.)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, everyting shoud be in english

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@achains achains self-requested a review August 15, 2023 15:27
.idea

#macOS
.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still can see .DS_Store in config directory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@LevDenisov LevDenisov changed the title Added benchmark-artifact Added benchmark Oct 23, 2023
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.

2 participants