MATLAB tests are broken again
The issue is case sensitivity of static methods after adding adjointMap upgrade, which (mea culpa) I only tested in python.
Comments (12)
-
-
reporter That would be amazing! Probably not the easiest to fix, though. Issue is that wrap somehow cant’t deal with both adjointMap and AdjointMap. One is static and other is not. I think the fix is a change in wrap c++ code.
-
Oh. Hmm. That does sound more challenging. I think I know which code you're talking about (I think I poked around in there when I was looking into serialization issues). I'll take a glance at it, but you're probably right that it would be pretty involved.
-
reporter Mike, did you make any headway with this. On the SLAM user group Jason indicated this affects much more than just the unit test, but kills the toolbox altogether :-(
-
Crap.
No, I started looking at the wrap code (statically -- not in the debugger) and it wasn't clear how to fiddle with the lexicographical pairings from C <-> MATLAB to respect capitalization differences. If it's broken that badly (I just read the thread in the user group), I should give it a real look.
My GTSAM fork has been frozen for a little bit because I'm working on something -- I didn't even notice the break until you reported it. I don't have a non-production machine I can screw around on right this second, but I should early this week, and I can look then.
So,just to be clear, you added
adjointMap
to the wrapper; the Cython side is okay, and just the MATLAB is broken? -
reporter That is correct. I'm thinking it can be fixed temporarily by renaming that method, which should fix the MATLAB wrapper for now, and then we can brainstorm about the longer-term solution, which is fixing wrap.
-
That sounds like a plan. Let me try to find a machine and I can test it out.
-
First of all, thank you for providing this useful library open-source. I am currently using it in combination with the GPMP2 toolbox, but because of this bug, I have not been able to use the corresponding GPMP2 matlab toolbox. I tried checking out older commits and building from them, but I haven't managed to build a version that does not have this issue. Therefore, if I may ask, could you give me a pointer to where this issue has been introduced/ which the best commit is to pull from that predates this issue. Also, could you give a rough indication on when this issue is planned to get fixed?
Kind regards,
Twan
-
Hi, @tvdsys . If you take 302978a , I think that is the last commit before the problem was introduced, which was probably with commit 1d97f86. If you pull and build from there, you will probably be fine (that's where I was for a while), and you won't miss too much.
I have a machine set up now where I can replicate the problem. I have two things I must take care of before I can actually work on this. I'm hoping to take a look at it next weekend. I miiight get to look at it before then.
-
Thank you for the fast response! I will check it out.
-
reporter I just created a PR that should fix this. Please try it, let me know of any issues, and I will merge into develop.
-
reporter - changed status to closed
Merged in fix/wrappers (pull request #380) close issue
#413close issue#421Fix/wrappers→ <<cset 2a0153cf2552>>
- Log in to comment
@dellaert Frank, do you expect that this is still busted? If so, I can take a look.