-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Use Cint to match C++'s int type, not Julia's Int. #2283
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
Conversation
|
Hi @rcurtin I want to ask that can we can Cdouble in place of Float64. |
|
@Yashwants19 correct me if I'm wrong, but it seems to me on https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#man-bits-types-1 like |
|
https://github.com/mlpack/mlpack/blob/f482383a8bb0f915829510879afa604cc514cabd/src/mlpack/bindings/julia/mlpack/cli.jl.in#L213 |
Sorry I think so I had overlooked this part. :) |
| -DARMADILLO_LIBRARY="..\armadillo-8.400.0\Release\armadillo.lib" ` | ||
| -DBOOST_INCLUDEDIR=$(Agent.ToolsDirectory)\boost.1.60.0.0\lib\native\include ` | ||
| -DBOOST_LIBRARYDIR=$(Agent.ToolsDirectory)\boost_libs ` | ||
| -DBUILD_JULIA_BINDINGS=OFF ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that is strange looks like we are still trying to run the julia bindings tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the debugging exposed a bug in the CMake configuration. 😄 I think 3f07bcf fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I guess with #2278 we should see more green builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's hoping! It's been a really bad week for our build systems and I think a good part of that is my fault. 😄
Nice catch! Thank you. Fixed in 4bb869a. |
zoq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
|
I will wait until Azure builds this time before merging. 😄 |
|
Hm, let's see if we can bring the azure build back. |
|
Manually started the build: https://dev.azure.com/mlpack/mlpack/_build/results?buildId=998&view=results not sure why the last commit hasn't triggered the build. |
favre49
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems OK to me
birm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see this resolved!
|
Looks like all the builds passed (besides MacOS) :). I'll go ahead and merge it |
I mistakenly thought that Julia's
Inttype always matched C/C++'sinttype; this is incorrect---Julia'sInttype instead matchessize_t. Therefore, we have to use theCinttype in Julia. This PR fixes that and all associated handling, and should fix the currently-failing Julia build.