Skip to content

# fix issue #1237#1288

Merged
jdumas merged 1 commit intolibigl:devfrom
stnoh:dev
Oct 17, 2019
Merged

# fix issue #1237#1288
jdumas merged 1 commit intolibigl:devfrom
stnoh:dev

Conversation

@stnoh
Copy link
Copy Markdown
Contributor

@stnoh stnoh commented Sep 16, 2019

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.

Hello, I tested the fix by @xinyazhang on #1230 and #1237 in my environment (VS2017/Windows 10). Unfortunately, it still doesn't work for me. I checked it with the debugger and found that Aeq is still empty after running the original line of code (maybe this should be Eigen bug).
To fix this, I made one additional line to create M.diagonal().transpose() and make it sparse representation in the second line. Additionally, I also set the type as Scalar, not "double" to follow the original template.
I certificated that it now works in my environment.

@stnoh
# fix issue
a65d3d2 
- make sure to create M.diagonal().transpose() as M_diag_tr
- using proper type: double -> Scalar
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Sep 16, 2019

There is definitely a Eigen bug to be reported there. I was waiting for someone to create a MWE and report the bug upstream before I merge the PR. We should also have a unit test to catch this use-case, to make sure we don't break it again inadvertently.

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 8, 2019

@stnoh @xinyazhang this has been hanging for a while. Has anyone produced a MWE and reported the bug upstream to Eigen?

@stnoh
Copy link
Copy Markdown
Contributor Author

stnoh commented Oct 8, 2019

Nope, I didn't because @xinyazhang said he was writing a bug report in #1230 ...
However, I still cannot find his report from Eigen Bugzilla, neither.
http://eigen.tuxfamily.org/bz/buglist.cgi?component=Sparse

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 8, 2019

It doesn't seem @xinyazhang ever reported it upstream, so maybe you can do it @stnoh? (Worst case we'll have it reported twice...).

@stnoh
Copy link
Copy Markdown
Contributor Author

stnoh commented Oct 8, 2019

Urgh... Do you have an Eigen bugzilla account, @jdumas ?
Just now I tried to report it, but their system does not permit self-registration.
Before reporting a bug, I need to send an e-mail for Bugzilla account request. But I'm not sure how much it will take time to get an account.

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 8, 2019

Yeah it says self-registration is disabled due to spam. But if you email the address shown on the webpage they should create an account pretty quickly. I think I got my account before they disabled self-registration due to spam.

@xinyazhang
Copy link
Copy Markdown

xinyazhang commented Oct 9, 2019 •
edited
Loading

It doesn't seem @xinyazhang ever reported it upstream, so maybe you can do it @stnoh? (Worst case we'll have it reported twice...).

Sorry for the delay, I'm not working on Windows for the most of my time. @stnoh @jdumas have you filed the report? I could file one tonight with my home laptop.

Fix: I'm NOT working on Windows for the most time

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 9, 2019 •
edited
Loading

I have not, and I'm working on Linux most of the time, so I would appreciate if one of you could do it. Thanks!

@xinyazhang
Copy link
Copy Markdown

xinyazhang commented Oct 9, 2019

@stnoh @jdumas submitted as https://eigen.tuxfamily.org/bz/show_bug.cgi?id=1754

@stnoh
Copy link
Copy Markdown
Contributor Author

stnoh commented Oct 10, 2019

Oh, I got Eigen account now, but @xinyazhang already reported a bug.
(In my case, I AM working in Windows for the most of time 😃)

@jdumas jdumas merged commit 28f20cd into libigl:dev Oct 17, 2019
This was referenced Oct 17, 2019
Closed
Closed
Closed
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 17, 2019

Hmm I realized I merged this but we still need to add a unit test to make sure we don't reopen the bug with a future update to Eigen. Would any of you Windows guys be willing to add that?

@stnoh
Copy link
Copy Markdown
Contributor Author

stnoh commented Oct 21, 2019

Sorry for late reply... I may write something, but where should I start?
I looked out some information from the repo, but it was slightly confusing, because #1059 meant some part of the instruction page (https://libigl.github.io/unit-tests/) is out-of-date...

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Oct 21, 2019

I would suggest to start from an existing unit test, e.g. this one (I picked randomly). Copy the structure and write your own test. Let me know if you have any question!

@BruegelN BruegelN mentioned this pull request Nov 13, 2019
Closed
4 tasks
@alecjacobson alecjacobson mentioned this pull request Nov 13, 2019
Merged
5 tasks
ShuangLiu1992 added a commit to ShuangLiu1992/libigl_embree that referenced this pull request Mar 9, 2020
@ShuangLiu1992
Update heat_geodesics.cpp
c21b562 
libigl/libigl#1288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdumas jdumas jdumas approved these changes

Assignees

No one assigned

Labels

eigen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stnoh @jdumas @xinyazhang