Skip to content

Some refine works related to range-based for-loop of map constainer #1502

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
Merged
merged 3 commits into from
Apr 13, 2023
Merged

Some refine works related to range-based for-loop of map constainer #1502

merged 3 commits into from
Apr 13, 2023

Conversation

zubingtan
Copy link
Contributor

@zubingtan zubingtan commented Mar 27, 2023 •
edited
Loading

This PR:

  1. reserve vector size in DecisionTreeFactor::apply
  2. use auto in range-base for-loop to avoid implictly conversion in VectorValues and DecisionTreeFactor. Some format issues are address, too (add spaces).

@zubingtan zubingtan changed the title use auto for map for-loop Some refine related to range-based for-loop of map constainer Mar 27, 2023
@zubingtan zubingtan changed the title Some refine related to range-based for-loop of map constainer Some refine works related to range-based for-loop of map constainer Mar 27, 2023
@zubingtan zubingtan force-pushed the use_auto_for_map_in_range_based_for_loop branch from 02d3144 to bea79ad Compare March 27, 2023 04:06
@zubingtan
Copy link
Contributor Author

zubingtan commented Mar 27, 2023 •
edited
Loading

@varunagrawal Can you help to take a look at the CI fails?
image

Or, do I have to wait for reviewer approval to get rid of those fails?

@zubingtan
Copy link
Contributor Author

@dellaert Can anyone help me to review this PR?

dellaert
dellaert approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! CI has to pass so I re-triggered windows CI run.

@dellaert
Copy link
Member

dellaert commented Apr 6, 2023

@zubingtan for some reason the windows CI failed. Could you merge in the latest develop and push?

@zubingtan
use auto for map for-loop
329041d 
1. reserve vector size in DecisionTreeFactor::apply
2. use auto in range-base for-loop to avoid implictly conversion in VectorValues and DecisionTreeFactor. Some format issues are address, too (add spaces).
@zubingtan zubingtan force-pushed the use_auto_for_map_in_range_based_for_loop branch from bea79ad to 329041d Compare April 8, 2023 06:54
@zubingtan
Copy link
Contributor Author

@zubingtan for some reason the windows CI failed. Could you merge in the latest develop and push?

@dellaert done. Could you help to tigger the CI pipeline again? thanks !

@dellaert
Copy link
Member

dellaert commented Apr 9, 2023

So weird. I can't re-trigger. Maybe this is because of force-push? Can you just add a "dummy commit" (maybe reformat something slightly) and just "push" not force-push? stumped...

@zubingtan
Copy link
Contributor Author

@dellaert Done.

And then I realized we really don't have this DefaultChart, right? Anyway, I searched gtsam and couldn't find it

D:\a\gtsam\gtsam\gtsam/base/chartTesting.h(37,39): error C2760: syntax error: unexpected token '<', expected 'declaration' [D:\a\gtsam\gtsam\build\gtsam\gtsam.vcxproj]

image

@dellaert
Copy link
Member

Right, but how does that only pop up now? @varunagrawal this is also a think in tour PR. any ideas? For a fix?

@zubingtan
Copy link
Contributor Author

Right, but how does that only pop up now? @varunagrawal this is also a think in tour PR. any ideas? For a fix?

How about I make a PR to delete chartTesting.h. It seems that nobody uses it.

@varunagrawal
Copy link
Contributor

I wouldn't recommend deleting chartTesting since that's a symptom not the cause.

My gut says that something changed in the Actions Windows image and that change is causing this issue. I'll take a closer look at it.

@zubingtan
Copy link
Contributor Author

I wouldn't recommend deleting chartTesting since that's a symptom not the cause.

My gut says that something changed in the Actions Windows image and that change is causing this issue. I'll take a closer look at it.

However, gtsam::DefaultChart<T> is not in the codebase now. Maybe someone deleted gtsam::DefaultChart<T> before and forgot to delete chartTesting...

@varunagrawal
Copy link
Contributor

@dellaert issue is related to a CMake command. I have added a fix at #1508

@dellaert
Copy link
Member

Merge in develop to re-run windows CI?

@zubingtan
Copy link
Contributor Author

Merge in develop to re-run windows CI?

Done for merge, please help to re-trigger the CI. thanks~

@varunagrawal varunagrawal merged commit 5ab98a8 into borglab:develop Apr 13, 2023
@zubingtan zubingtan deleted the use_auto_for_map_in_range_based_for_loop branch April 13, 2023 05:52
@asmei1 asmei1 mentioned this pull request Aug 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dellaert dellaert dellaert 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.
3 participants
@zubingtan @dellaert @varunagrawal