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

fuse -> ROS 2 fuse_loss: Port fuse_loss #287

Merged
merged 6 commits into from
Dec 3, 2022
Merged

fuse -> ROS 2 fuse_loss: Port fuse_loss #287

merged 6 commits into from
Dec 3, 2022

Conversation

methylDragon
Copy link
Collaborator

@methylDragon methylDragon commented Nov 17, 2022 •
edited
Loading

See: #276

Description

This PR ports the entirety of fuse_loss, including:

  • CMakeLists and packages
  • Source code
  • Tests

It makes minor changes to fuse_core to support the loss changes.
Also note that some changes to fuse_model were made (that won't allow it to get built, but are put in for posterity.)

Other stuff like docs are in other PRs

Notes

Pinging @svwilliams for visibility.

@methylDragon methylDragon mentioned this pull request Nov 17, 2022
Closed
54 tasks
@methylDragon methylDragon force-pushed the rolling-loss branch 2 times, most recently from 6498b04 to 3b56294 Compare November 18, 2022 00:08
@methylDragon methylDragon mentioned this pull request Nov 18, 2022
Merged
@methylDragon methylDragon mentioned this pull request Nov 18, 2022
Merged
@methylDragon methylDragon force-pushed the rolling-loss branch 2 times, most recently from 9245bc0 to 2d3ac06 Compare November 18, 2022 23:34
sloretz
sloretz reviewed Nov 22, 2022
View reviewed changes
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Mind fixing this compiler warning?

In file included from /home/osrf/ws/rolling/src/fuse/fuse_loss/test/test_qwt_loss_plot.cpp:46:
/home/osrf/ws/rolling/src/fuse/fuse_loss/include/fuse_loss/qwt_loss_plot.h: In constructor ‘fuse_loss::QwtLossPlot::QwtLossPlot(const std::vector<double, std::allocator<double> >&, const fuse_loss::HSVColormap&)’:
/home/osrf/ws/rolling/src/fuse/fuse_loss/include/fuse_loss/qwt_loss_plot.h:221:35: warning: ‘static QVector<T> QVector<T>::fromStdVector(const std::vector<BufferT>&) [with T = double]’ is deprecated: Use QVector<T>(vector.begin(), vector.end()) instead. [-Wdeprecated-declarations]
  221 |     : residuals_(QVector<double>::fromStdVector(residuals))

colcon build --packages-select fuse_loss --cmake-args -DBUILD_WITH_PLOT_TESTS=ON -DBUILD_WITH_INTERACTIVE_TESTS=ON

fuse_loss/CMakeLists.txt Outdated
)

install(
FILES fuse_plugins.xml
DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}
DESTINATION share/${PROJECT_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use pluginlib_export_plugin_description_file() https://github.com/ros/pluginlib/blob/rolling/pluginlib/cmake/pluginlib_export_plugin_description_file.cmake - it will both do this install and setup the right stuff in the ament_index for pluginlib to find the plugins.

fuse_loss/CMakeLists.txt Outdated

add_library(${PROJECT_NAME}
## fuse_loss library
add_library(${PROJECT_NAME} SHARED
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you intentionally leave off SHARED then this library can be built statically by setting -DBUILD_SHARED_LIBS=OFF in CMake.

This is one of those weird CMake features/conventions, but It looks like it was being used here before since SHARED wasn't specified. ament_cmake_ros documents that option when it's find_package()'d, so change find_package(ament_cmake REQUIRED) to find_package(ament_cmake_ros REQUIRED) above too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll also remove the SHARED on fuse_core in this PR too. It was added by me in a previous PR.

fuse_loss/cmake/FindQWT.cmake
endif ()
include( FindPackageHandleStandardArgs )
if( CMAKE_VERSION LESS 2.8.3 )
find_package_handle_standard_args( QWT DEFAULT_MSG QWT_LIBRARY QWT_INCLUDE_DIR _QWT_VERSION_MATCH )
Copy link
Collaborator

Choose a reason for hiding this comment

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

The capitalization might be important here. I think it would be the difference between QWT_FOUND vs Qwt_FOUND being set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the capitalization so ament_lint_cmake would stop throwing warnings, is there a way to silence the linting for this? :o

Copy link
Collaborator Author

@methylDragon methylDragon Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

According to the file itself, the variable that's being set is

#  QWT_FOUND - the system has Qwt

So I think we can change it? The error's coming from the CMAKE_MODULE_PATH call.
I've updated it to:
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../cmake")

fuse_loss/test/CMakeLists.txt Outdated

# Find Qwt using FindQwt.cmake copied from:
# https://gitlab.kitware.com/cmake/community/-/wikis/contrib/modules/FindQwt
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path is incorrect since it was moved to the test directory.

@methylDragon
Copy link
Collaborator Author

Addressed!
I also tried to roll in the same sort of changes requested in fuse_core: a211ec6

@sloretz
Copy link
Collaborator

sloretz commented Dec 2, 2022

@ros-pull-request-builder retest this please

@methylDragon
Port CMakeLists and package.xml
4f37c76 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Port fuse_loss
6da1eeb 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Port usages of loadLossConfig from fuse_models
8ee7ac6 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Update package.xml and lint CMake files
a3c4c0f 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Refine fuse_loss tests and address deprecation warnings
990a8a6 
Signed-off-by: methylDragon <methylDragon@gmail.com>
sloretz
sloretz reviewed Dec 2, 2022
View reviewed changes
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM with one small fix

All tests pass locally except cpplint, uncrustify, and copyright, but that's expected to be fixed in the next PR.

fuse_loss/package.xml Outdated
@@ -11,19 +11,26 @@
<author email="swilliams@locusrobotics.com">Stephen Williams</author>
<license>BSD</license>

<buildtool_depend>catkin</buildtool_depend>
<buildtool_depend>ament_cmake</buildtool_depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit ament_cmake_ros instead of ament_cmake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/locusrobotics/fuse/compare/25e6567ae3925ff56f5db18a4ea8d3dbb8e320f1..7e4da3d38f24cb4f39f0536c33531388fb297692

@methylDragon
Update Loss interface and fix tests
7e4da3d 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit bcd20f7 into locusrobotics:rolling Dec 3, 2022
@methylDragon methylDragon deleted the rolling-loss branch December 3, 2022 05:03
@sloretz sloretz mentioned this pull request Dec 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@methylDragon @sloretz