Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Changes to build the examples under MSVC#1

Open
mcleary wants to merge 4 commits intocodeplaysoftware:mastercodeplaysoftware/SYCL-ML:masterfrom
mcleary:windows-buildmcleary/SYCL-ML:windows-buildCopy head branch name to clipboard
Open

Changes to build the examples under MSVC#1
mcleary wants to merge 4 commits intocodeplaysoftware:mastercodeplaysoftware/SYCL-ML:masterfrom
mcleary:windows-buildmcleary/SYCL-ML:windows-buildCopy head branch name to clipboard

Conversation

@mcleary
Copy link

@mcleary mcleary commented Dec 25, 2017

The FindComputeCpp.cmake was updated to the latest version.
The download mechanism for the mnist database was changed to use CMake file command.
The file smo.hpp had some incompatibilities with MSVC.

Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the addition to the eigen.patch I think we should push them in Eigen instead. I'd like to keep a patch file as small as possible.
Do you want to submit them yourself on Eigen? The need of std::remove_reference is definitely something to investigate with Mehdi. I can take care of this later if you can't spend time on this.

endforeach()

# Need to define _USE_MATH_DEFINES for M_PI when building on MSVC
if(MSVC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove the usage of M_PI so you can remove that.

foreach(SOURCE ${SOURCES})
process_source_name(${SOURCE})
add_executable(${TARGET_NAME} ${SOURCE})
add_executable(${TARGET_NAME} ${SOURCE})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the spaces here?


if (max_nb_iter == 0)
max_nb_iter = std::max(10000000LU, m > INT_MAX/100 ? INT_MAX : 100*m);
max_nb_iter = std::max(10000000ULL, m > std::numeric_limits<size_t>::max()/100ULL ? std::numeric_limits<size_t>::max() : 100ULL*m);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break on Unix since the index type is unsigned long there. I'd like to know why it is long long on Windows, maybe Mehdi knows more about this.
If possible I'd like to have a clean solution on the Eigen side for this, if it's not possible I'll use decltype instead here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t is a long long on Windows. You could do a static cast into the size_t type but that seems ugly. You should probably just make sure that all the types in that call are the same, whatever they might be. I guess m might be the problem here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, that's annoying. m is part of the problem yes, I have 2 constants that should be of type size_t (for min and max type deduction). It's not great that I have these constants but that's an issue for another time, I'll have to static_cast them for now.
Thanks!

@mcleary
Copy link
Author

mcleary commented Dec 26, 2017

My mistake on the eigen.patch. I will restore the patch to the original version.

@Rbiessy
Copy link
Contributor

Rbiessy commented Dec 26, 2017

It's alright. At least people can see what needs to be done if they want to try to make it work as well :)
Hopefully, we'll have a cleaner solution soon.

@mcleary
Copy link
Author

mcleary commented Feb 19, 2019

@Rbiessy is this patch still relevant? If it is let me know and I will update it.

@Rbiessy
Copy link
Contributor

Rbiessy commented Feb 21, 2019

Well I would definitely want to merge it if I had time to work on this project but I don't think it will happen.
The changes in smo.hpp are the only issue I see. I would need to clean the usage of constants and test again on Linux and Windows... Do you want to close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.