Skip to content

spectra module#2216

Merged
alecjacobson merged 7 commits intomainfrom
alecjacobson/spectra
Jun 28, 2023
Merged

spectra module#2216
alecjacobson merged 7 commits intomainfrom
alecjacobson/spectra

Conversation

@alecjacobson
Copy link
Copy Markdown
Contributor

@alecjacobson alecjacobson commented Jun 25, 2023

This introduces a spectra module. I'm still not 100% convinced that spectra is reliable. This uses a shift-invert mode with Eigen's SparseLU as a solver.

Matlab uses Arpack + umfpack. Eigen does provide a wrapper for umfpack, which would mean further including a SuiteSparse dependency (I'd say this is out of scope for now). So, in the meantime, I've templated over the solver so if a user really wanted to use umfpack they could.

One thing that makes me uncomfortable is that spectra did not find the correct modes for a basic unit test without really increasing the shift value sigma. Ideally this would work with sigma=0 or some other reliable procedure that is hidden from the caller. I'm not sure if we should blame spectra or SparseLU or the way I'm calling spectra for this issue.

As a use-case, I included Spectral Conformal Parametrization. Another great addition would be the skinning eigenmodes from https://arxiv.org/abs/2303.11886.

@alecjacobson
Copy link
Copy Markdown
Contributor Author

alecjacobson commented Jun 25, 2023 

CMake Error at build/_deps/spectra-src/CMakeLists.txt:24 (find_package):
  Could not find a package configuration file provided by "Eigen3" with any
  of the following names:

    Eigen3Config.cmake
    eigen3-config.cmake

  Add the installation prefix of "Eigen3" to CMAKE_PREFIX_PATH or set
  "Eigen3_DIR" to a directory containing one of the above files.  If "Eigen3"
  provides a separate development package or SDK, be sure it has been
  installed.

Huh. I'm not seeing this problem locally. If anyone knows a quick fix I'd be grateful, I won't have time to return to this for a few days.

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jun 25, 2023

Huh. I'm not seeing this problem locally. If anyone knows a quick fix I'd be grateful, I won't have time to return to this for a few days.

I guess that's because you have a system-wide version of Eigen that Spectra picks up. Spectra is looking for Eigen via find_package() without checking that the target already exist first. I think you can set Eigen3_FOUND to true before including Spectra to trick it into thinking it has found Eigen (since it uses the target Eigen3::Eigen later on this will work).

@alecjacobson
Copy link
Copy Markdown
Contributor Author

alecjacobson commented Jun 26, 2023

Thanks, @jdumas . Unfortunately, that doesn't seem to work. I'm not sure how I can trick spectra into seeing our Eigen.

@alecjacobson
Copy link
Copy Markdown
Contributor Author

alecjacobson commented Jun 26, 2023

It looks like maybe this pull request on spectra would solve our issues?

https://github.com/yixuan/spectra/pull/127/files

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jun 26, 2023

Yes it would definitely solve the issues we're having. I don't understand why it's been opened for 2y...

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jun 26, 2023

If they don't merge such a change soon I can work on a libigl-side workaround later this week, I've done this a million times for misbehaving projects...

@alecjacobson
Copy link
Copy Markdown
Contributor Author

alecjacobson commented Jun 26, 2023

Thanks, that'd be great. Using my fork is not a great solution. Though it seems spectra is not changing very frequently.

@alecjacobson
Copy link
Copy Markdown
Contributor Author

alecjacobson commented Jun 28, 2023

Merging this in for now. If spectra merges yixuan/spectra#152 or we find a cmake work around. We can get rid of the alecjacobson mirror url.

@alecjacobson alecjacobson merged commit b1bd5b1 into main Jun 28, 2023
@alecjacobson alecjacobson deleted the alecjacobson/spectra branch June 28, 2023 15:37
BruegelN added a commit to BruegelN/libigl that referenced this pull request Jul 18, 2023
@BruegelN
Cleanup libigl#2216
b65bbde
alecjacobson pushed a commit that referenced this pull request Aug 17, 2023
@BruegelN
Cleanup #2216 (#2225)
3cf08b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alecjacobson @jdumas