Change matrices and other fixes for python bindings by teseoch · Pull Request #1162 · libigl/libigl

teseoch · 2019-04-18T15:21:02Z

[Describe your changes and what you've already done to test it.]

Check all that apply (change to `[x]`)

All changes meet libigl style-guidelines.
Adds new .cpp file.
Adds corresponding unit test.
Adds corresponding python binding.
This is a minor change.

fwilliams · 2019-04-18T15:27:54Z

include/igl/bfs.cpp

  typename PType>
 IGL_INLINE void igl::bfs(
-  const Eigen::SparseMatrix<AType> & A,
+  const AType & A,


Are we sure we can't use SparseCompressedBase?

fwilliams · 2019-04-18T15:28:08Z

include/igl/bfs.h

    typename PType>
  IGL_INLINE void bfs(
-    const Eigen::SparseMatrix<AType> & A,
+    const AType & A,


Same comment as above

fwilliams · 2019-04-18T15:29:09Z

include/igl/arap_linear_block.h

    const int d,
    const igl::ARAPEnergyType energy,
-    Eigen::SparseMatrix<Scalar> & Kd);
+    MatK & Kd);


Can these be either SparseMatrixBase or SparseCompressedBase?

Note: I think we'll need to bump the version of Eigen for this.

doesnt work. SparseCompressedBase does not support resize reserve etc

fwilliams · 2019-04-18T15:30:02Z

include/igl/arap_rhs.cpp

+    const Eigen::MatrixBase<DerivedF> & F,
+    const int dim,
+    const igl::ARAPEnergyType energy,
+    Eigen::SparseMatrixBase<DerivedK>& K)


When do we use SparseMatrixBase and when do we use SparseCompressedBase?

fwilliams · 2019-04-18T15:30:30Z

include/igl/arap_rhs.h

    const int dim,
    const igl::ARAPEnergyType energy,
-    Eigen::SparseMatrix<double>& K);
+    Eigen::SparseMatrixBase<DerivedK>& K);


When do we use SparseMatrixBase and when do we use SparseCompressedBase?

fwilliams · 2019-04-18T15:31:57Z

include/igl/colon.cpp

@@ -17,7 +17,7 @@ IGL_INLINE void igl::colon(
  const H hi,


We don't have to do this, but these should all be Eigen::Index

fwilliams · 2019-04-18T15:40:26Z

include/igl/vertex_components.cpp

+template <typename SparseT, typename DerivedC, typename Derivedcounts>
 IGL_INLINE void igl::vertex_components(
-  const Eigen::SparseMatrix<AScalar> & A,
+  const SparseT & A,


SparseCompressedBase

fwilliams · 2019-04-18T15:40:35Z

include/igl/vertex_components.cpp

+template <typename SparseT, typename DerivedC>
 IGL_INLINE void igl::vertex_components(
-  const Eigen::SparseMatrix<AScalar> & A,
+  const Eigen::SparseCompressedBase<SparseT> & A,


.travis.yml

cmake/libigl.cmake

jdumas

Wait so this commit is removing the old python bindings, but is not adding the new ones?

fwilliams · 2019-04-26T19:16:26Z

@jdumas yes. Right now we're working on the new python bindings in a separate repo which has libigl as a cmake external project. We're making changes to libigl and sending PRs (like this one) as we go along.

@teseoch ran into some tricky compilation issues with the old bindings. We realized it would be much less effort if we killed the old bindings in this repository while we make changes necessary for the new bindings (as opposed to fixing the old bindings to work with the new changes).

If others are using the python bindings in the dev branch, then we could move the old python bindings to a seperate repo with libigl as a CMake external project and point the external project version to the commit before this one.

jdumas · 2019-04-26T19:27:59Z

People are still using the dev branch with the old python bindings. The old python bindings are currently broken in master, and I think we should maintain a version of libigl with the old bindings until the new ones are ready for general use.

Why do the new python bindings use libigl as an external project? Wouldn't it be better to have them in the libigl main repo (we had this discussion before), as it would prevent them from using a deprecated version of libigl and make sure new PRs are not breaking existing python bindings.

jdumas · 2019-04-26T19:30:22Z

If anything, I would be more comfortable pushing the current dev branch into master before we merge this commit, and maybe create a release branch if we need to backport fixes until the new bindings are ready.

fwilliams · 2019-04-26T19:55:55Z

We decided with @danielepanozzo and @teseoch to have the python bindings in a separate repository while we develop them. This let's us "move fast and break things" in the binding code as well as have a small group of people have direct push access for bindings. Necessary changes to libigl are sent as pull requests as we go along and if we merge things fast enough, then we can keep everything up to date.

In contrast if we were to directly develop against dev, every small change to the bindings will require a PR and a lot more overhead.

We can decide later if we want to move the bindings into the main repo or not (there are pros and cons both ways and I don't really have an opinion on this tbh), but at least for now the setup we have makes sense if we want to get things done.

Pushing the current dev into master sounds like a great idea if its ready. How quickly can we do this by? I would really like to land this pull request. We're going to have a lot more changes like this and the longer they hang, the more difficult it's going to be to merge everything.

jdumas · 2019-04-26T20:05:32Z

Well, this depends on @alecjacobson and @danielepanozzo more than me. But before merging dev into master, we should take a look at other existing -- smaller -- pull-requests, and see if they can be merged. Then we need to update the changelog on the website with the list of new features, as we merge into master and tag the new release.

I thought we had created the dev-python exactly for the purpose of developing the new python bindings into libigl, since it is not protected, and can be kept in sync with master/dev as needed. It would be weird to turn dev into an unstable branch were python bindings are nuked until new ones are created. At least I'm worried it might give the wrong impression if this drags on.

teseoch · 2019-05-03T23:57:34Z

something is wierd with appveyor, any idea?

jdumas · 2019-05-04T00:32:56Z

It probably means you need to resolve the conflicts against dev before AppVeyor can build again.

Teseo Schneider and others added 9 commits April 12, 2019 12:21

fixed arap

eb31a3d

avg vertices

bb01d2a

stuff, test not passing

8a7a4d6

add template instantiations for unary op

3ef29f9

uncomment tests

65a7c43

derp

a926eeb

fixed matrix types

64db6fd

fixed volume size

1b75495

template stuff

10af4dc

fwilliams reviewed Apr 18, 2019

View reviewed changes

Teseo Schneider and others added 20 commits April 18, 2019 16:25

bump eigen verion and sparse matrices fix

73ce4ec

missing templates

c0aeeb7

template instantiations

1755db7

template instantiations

d0e62a8

template instantiations

c4c6efe

Merge branch 'dev' into dev

95440f1

removed old python binding

a51b697

fixed arap

364b0b1

avg vertices

738c6a3

stuff, test not passing

11b32cf

add template instantiations for unary op

7235398

uncomment tests

19729b8

derp

de397ca

fixed matrix types

08abe36

fixed volume size

0c7a950

template stuff

2a38bac

bump eigen verion and sparse matrices fix

9126914

missing templates

bbc8903

template instantiations

7dc917b

template instantiations

416e51b

Teseo Schneider added 2 commits April 24, 2019 13:46

removed old python binding

53c5a9c

Merge branch 'dev' of github.com:skoch9/libigl into dev

05e0df7

teseoch requested a review from fwilliams April 26, 2019 14:01

fwilliams approved these changes Apr 26, 2019

View reviewed changes

jdumas reviewed Apr 26, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

jdumas reviewed Apr 26, 2019

View reviewed changes

cmake/libigl.cmake Show resolved Hide resolved

jdumas requested changes Apr 26, 2019

View reviewed changes

Teseo Schneider added 2 commits May 3, 2019 19:36

harmonic templates for python

feb441f

fixed cmake

aa04a74

fixing templates

0704220

jdumas added breaking-change python labels May 6, 2019

Teseo Schneider and others added 4 commits May 28, 2019 11:20

fixed templates on bone parents

9d8aa1c

fixed ear

c39303d

edited writeOFF

e62471a

removed old new binding

958392c

teseoch closed this Jun 14, 2019

teseoch reopened this Jun 14, 2019

Merge branch 'skoch9-dev' into dev

8624d84

teseoch closed this Jun 14, 2019

teseoch reopened this Jun 14, 2019

teseoch mentioned this pull request Jun 14, 2019

Python #1228

Merged

teseoch closed this Jun 14, 2019

Conversation

teseoch commented Apr 18, 2019

Check all that apply (change to [x])

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jdumas left a comment

Choose a reason for hiding this comment

Uh oh!

fwilliams commented Apr 26, 2019

Uh oh!

jdumas commented Apr 26, 2019

Uh oh!

jdumas commented Apr 26, 2019

Uh oh!

fwilliams commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdumas commented Apr 26, 2019

Uh oh!

teseoch commented May 3, 2019

Uh oh!

jdumas commented May 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Check all that apply (change to `[x]`)

fwilliams commented Apr 26, 2019 •

edited

Loading