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

fix stof error #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix stof error #5

wants to merge 3 commits into from

Conversation

JunseoMin
Copy link

Hi , @LimHyungTae

Thank you for all your amazing contributions! I’ve been following your work on GitHub, LinkedIn, and through your papers.

While running ORORA with the Mulran dataset (sequence DDC01), I encountered an error in the loadGTPoses function within include/SE2posemanager.hpp. The error was:

terminate called after throwing an instance of 'std::out_of_range' what(): stof

After some investigation, I discovered that the issue occurs because the radar_odometry.csv file contains the value 1.43493e-42. To handle this properly, I changed the conversion from stof to stod.

Additionally, I noticed that the vector parsed_xyyaw was mistakenly parsing the yaw value twice (using words[3] for both y and yaw). I corrected it so that it now properly reads x, y, and yaw:

std::vector<float> parsed_xyyaw = {stod(words[1]), stod(words[2]), stod(words[3])};

I’ve also added a few safety functions to improve robustness.

Thank you for taking the time to review these changes. I’m really excited to contribute to a codebase by someone I truly admire!

Best regards,
Junseo Min

Copy link
Member

@LimHyungTae LimHyungTae left a comment

Choose a reason for hiding this comment

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

@JunseoMin Oh man appericate you having your warm and kind attention!

Could you consider my comments and push a commit again?

while (std::getline(ifs, line)) {
if (line.empty()) continue;
Copy link
Member

Choose a reason for hiding this comment

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

Does this situation occur in the current pipeline?

Copy link
Author

Choose a reason for hiding this comment

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

Nope. This situation does not occur in the current pipeline.

while (std::getline(ifs, line)) {
if (line.empty()) continue;
float x,y,yaw;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to declare unused variables here?

Copy link
Author

Choose a reason for hiding this comment

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

These two lines (28,29) were only used for debugging, and I accidentally forgot to remove them. Thanks for checking!


if (words.size() < 4) {
std::cerr << "Skipping invalid line: " << line << std::endl;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay to use continue? Wouldn't it be better to throw std::logic_error and provide guidance to the user on what went wrong?

@LimHyungTae
Copy link
Member

Again, thanks for your attention, @JunseoMin! By any chance, do you plan to use ORORA in ROS2 in your lab? If so, I'm always open to discussing it!

@JunseoMin
Copy link
Author

Thanks for reviewing! I'm interested in radar single-shot localization, but I need to discuss it with my professor first. (I'm currently doing an internship.) If I start researching with ORORA, can I reach out to you via email?

@JunseoMin
Copy link
Author

As a kind of fan of yours, I really appreciate your response!

For summery:

  1. I removed lines 28 and 29, and no empty lines appeared in the current pipeline.
  2. I added a logic error message: 'Invalid line detected from GT dataset!'

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