Skip to content

Conversation

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Mar 12, 2020

I mistakenly thought that Julia's Int type always matched C/C++'s int type; this is incorrect---Julia's Int type instead matches size_t. Therefore, we have to use the Cint type in Julia. This PR fixes that and all associated handling, and should fix the currently-failing Julia build.

@rcurtin rcurtin added this to the mlpack 3.3.0 milestone Mar 12, 2020
@Yashwants19
Copy link
Member

Hi @rcurtin I want to ask that can we can Cdouble in place of Float64.

@rcurtin
Copy link
Member Author

rcurtin commented Mar 12, 2020

@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 Float64 and Cdouble are entirely equivalent types on every system. (At least according to that table...)

@Yashwants19
Copy link
Member

@Yashwants19
Copy link
Member

@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 Float64 and Cdouble are entirely equivalent types on every system. (At least according to that table...)

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 `
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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. 😄

@rcurtin
Copy link
Member Author

rcurtin commented Mar 12, 2020

https://github.com/mlpack/mlpack/blob/f482383a8bb0f915829510879afa604cc514cabd/src/mlpack/bindings/julia/mlpack/cli.jl.in#L213
I think so here we are passing size_t from julia but accept const int from c++ correct if i am wrong.

Nice catch! Thank you. Fixed in 4bb869a.

Copy link
Member

@zoq zoq left a 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.

@rcurtin
Copy link
Member Author

rcurtin commented Mar 13, 2020

I will wait until Azure builds this time before merging. 😄

@zoq
Copy link
Member

zoq commented Mar 13, 2020

Hm, let's see if we can bring the azure build back.

@zoq
Copy link
Member

zoq commented Mar 13, 2020

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.

Copy link
Member

@favre49 favre49 left a 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

Copy link
Member

@birm birm left a 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!

@favre49
Copy link
Member

favre49 commented Mar 13, 2020

Looks like all the builds passed (besides MacOS) :). I'll go ahead and merge it

@favre49 favre49 merged commit 5676942 into mlpack:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants