Skip to content
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

No support for building on windows without MinGW #67

Closed
R-Goc opened this issue Jul 7, 2024 · 11 comments
Closed

No support for building on windows without MinGW #67

R-Goc opened this issue Jul 7, 2024 · 11 comments

Comments

@R-Goc
Copy link
Contributor

R-Goc commented Jul 7, 2024

i tried building with clang, and it mostly works. However, one of the graph test cases fails. When it gets weird though is when I try to include the library in another C file, and then i get an error for the use of an undeclared identifier arc4random, which does make sense. However, how I'm able to build the library I don't know.
Is there any reason for the use of platform specific code here?

Copy link

github-actions bot commented Jul 7, 2024

Thank you for your interest in AlgoPlus, a maintainer will see your issue soon!

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

Quick note, tried again with w64devkit version of MinGW also doesn't work. Edit: even MSYS mingw doesn't work.

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

A quick change to kmeans.h that got everything compiling:
kmeans(std::vector<std::vector> data, int K, int64_t MAX_ITER = 1500)
: data(data), K(K) {

std::random_device rd;
std::mt19937 gen(rd());
for (int i = 0; i < K; i++) {
  int rand_num = gen() % data.size() - 1;
  this->cluster_centers.push_back(data[rand_num]);
}

You should also provide a clang format file with the library, as my clang format on save broke the whole indentation.

@spirosmaggioros
Copy link
Member

spirosmaggioros commented Jul 7, 2024

Hi @R-Goc. Yes i agree with you that we actually never had a contributor with windows and there might be problems with building it. We once tried to build with windows 4-5 months ago and we came up with the build commands that you see on the README file.

I will look at the arc4random() error later in the day, it was a clang-tidy recommendation but it might need what you proposed as well(i made a couple of changes yesterday). If that was the only error, you can either make a PR or i will fix it later.

I guess we might need to add windows build automation as well to make sure everything works everytime, unfortunately I don't have windows, but we will see what we're going to do.

Than you for pointing this out!

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

Yeah, also just noticed that you're pushing ints into an array of doubles. So I'd change that as well, to actually push doubles. That would be even better. Clang tidy has some merit in what it is saying, if it was about replacing rand or random. However, there are better ways about it from what I know.

@spirosmaggioros
Copy link
Member

These errors might exist somewhere, i wrote almost everything myself and I'm adding things everyday to check the boxes. That's why it's opensource so people can point out mistakes and fix them.

The more people getting involved, the better.

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

Okay, what I have now:

kmeans(std::vector<std::vector> data, int K, int64_t MAX_ITER = 1500)
: data(data), K(K) {

std::random_device rd;
std::mt19937_64 gen(rd());
std::uniform_real_distribution<> distrib(
    std::numeric_limits<double>::min(), std::numeric_limits<double>::max());
for (int i = 0; i < K; i++) {
  double rand_num = distrib(gen);
  this->cluster_centers.push_back(data[rand_num]);
}

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

Why was there a limit of the number to the size of data? Will it break anything if there isn't one? Tests seem to pass.

@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 7, 2024

I can make a fork and a PR tomorrow, also here is the test that doesn't pass on clang, but passes on mingw

REQUIRE( g.topological_sort() == v1 )
with expansion:
{ 0, 2, 1, 3, 4 } == { 2, 0, 1, 3, 4 }

===============================================================================
test cases: 197 | 196 passed | 1 failed
assertions: 458 | 457 passed | 1 failed

@spirosmaggioros
Copy link
Member

spirosmaggioros commented Jul 7, 2024

Yes you can make a PR and propose the changes you want.

If you use a different framework or compiler, code will act differently. For example, a graph might have multiple topological sortings, because it depends on how the queue will prioritize the node insertion, but for me when i checked the code, it was returning this particular one, so i sticked with it.

But yes, i can add some ORs on this test case.

@R-Goc R-Goc closed this as completed Jul 8, 2024
@R-Goc
Copy link
Contributor Author

R-Goc commented Jul 8, 2024

Solved with #68

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

No branches or pull requests

2 participants