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

Removed minkindr gtsam (moved to ethz-asl/curves) #32

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

helenol
Copy link
Contributor

@helenol helenol commented Aug 12, 2015

Co-dependent on ethz-asl/curves#55

@gawela, @rdube, @mikebosse: make sure this is okay with you?

@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

@HannesSommer
Copy link
Contributor

I don't see this package belonging closer to curves than to minkindr (each as repositories). In fact I would vote for having it in a separate repository if it needs to get moved out of minkindr.
But if having this as an extra package within the minkindr repository causes troubles, maybe the problem is much more in the logic that decides which package to build.
In particular I don't see why somebody would intentionally build minkindr_gtsam if gtsam is not used. Can somebody point me to a representative problem case?

@HannesSommer
Copy link
Contributor

Okay, sorry, just found #31. Still digesting..

@helenol
Copy link
Contributor Author

helenol commented Aug 12, 2015

It's quite normal to build the whole workspace, especially when there are many binaries that are run (i.e., robot with 6+ different nodes running).
minkindr is a public repo/library that is meant to represent transformations, and is useful in many applications for representing robot pose/etc. without any reliance on higher-level libraries. minkindr_gtsam contains gtsam wrappers for minkindr types and is only relevant if you want to use gtsam, and brings in all the gtsam dependencies.
The specific example is we would like to package this, for example, with the other code for euroc on multirotors as a basic library of MAV nodes and accessory libraries. Bringing in the gtsam dependency any time we want to rely on the library makes no sense, and forcing anyone checking out our code to never be able to build the entire workspace (when often 5+ different binaries need to be built) is also not an option.

I am also happy to split this into another package if anyone other than curves is using it or you forsee anyone other than curves using it in the future, but I think it does not belong in the minkindr package.

@mikebosse
Copy link
Contributor

We should just make minkindr_gtsam its own project.

-Mike

On Wed, Aug 12, 2015 at 9:39 AM, Helen Oleynikova notifications@github.com
wrote:

It's quite normal to build the whole workspace, especially when there are
many binaries that are run (i.e., robot with 6+ different nodes running).
minkindr is a public repo/library that is meant to represent
transformations, and is useful in many applications for representing robot
pose/etc. without any reliance on higher-level libraries. minkindr_gtsam
contains gtsam wrappers for minkindr types and is only relevant if you want
to use gtsam, and brings in all the gtsam dependencies.
The specific example is we would like to package this, for example, with
the other code for euroc on multirotors as a basic library of MAV nodes and
accessory libraries. Bringing in the gtsam dependency any time we want to
rely on the library makes no sense, and forcing anyone checking out our
code to never be able to build the entire workspace (when often 5+
different binaries need to be built) is also not an option.

I am also happy to split this into another package if anyone other than
curves is using it or you forsee anyone other than curves using it in the
future, but I think it does not belong in the minkindr package.


Reply to this email directly or view it on GitHub
#32 (comment).

@HannesSommer
Copy link
Contributor

Thanks @helenol for the explanation. I totally see that nobody wants an accidental dependency on a beast like gtsam :).

Considering your arguments I would advocate for a separate repository (minkindr_gtsam, which can easily be open source as well so no extra cost for the lab). It just does not belong to curves. It is required / very valuable for any package using gtsam to optimize minkindr objects (both public). And they are more likely than packages depending on curves, which is private.

@HannesSommer
Copy link
Contributor

Who will create and setup the new repo (incl. jenkins)? I can jump in if nobody would like to volunteer ;).

@mikebosse
Copy link
Contributor

Yes Hannes, if you can set up the new repo, that would be great!

On Wed, Aug 12, 2015 at 10:28 AM, Hannes Sommer notifications@github.com
wrote:

Who will create and setup the new repo (incl. jenkins)? I can jump in if
nobody would like to volunteer ;).


Reply to this email directly or view it on GitHub
#32 (comment).

@helenol
Copy link
Contributor Author

helenol commented Aug 12, 2015

@HannesSommer: Sweet, thanks a lot! That sounds perfect. :) Thanks for taking point on this.

@HannesSommer
Copy link
Contributor

Almost done. I'm still fighting with the solution for issue #31 for make jenkins happy with the new https://github.com/ethz-asl/minkindr_gtsam/. Progress is blocked by ethz-asl/lab-infrastructure#2.
How urgent is it for you, @helenol ?
I would vote for an optimistic merge here. Fixing the dependers of minkindr_gtsam. Is easier then.

@helenol
Copy link
Contributor Author

helenol commented Aug 12, 2015

@HannesSommer: not urgent. Don't have access to the lab infrastructure repo so no clue what the issue is there, but I am happy to wait or merge this now. :) Merge as you please.

HannesSommer added a commit that referenced this pull request Aug 12, 2015
Removed minkindr gtsam (moved to ethz-asl/curves)
@HannesSommer HannesSommer merged commit f69d7c1 into master Aug 12, 2015
@HannesSommer HannesSommer deleted the feature/move-minkindr-gtsam branch August 12, 2015 12:56
@HannesSommer
Copy link
Contributor

@gawela, @mikebosse and @rdube please don't forget to checkout git@github.com:ethz-asl/minkindr_gtsam.git as soon as you pull in minkindr's master.

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.

4 participants