libscran / mnncorrect

C++ implementation of the MNN correction algorithm
https://libscran.github.io/mnncorrect/
MIT License
4 stars 0 forks source link

OpenMP for-loop index must be signed in Windows #10

Open GregReese219 opened 1 year ago

GregReese219 commented 1 year ago

In Microsoft's implementation of OpenMP, the index variable of an OpenMP for-loop must be a signed integer. (See Section 2.4.1 of this documentation). Thus, for example, lines 39-40 of CustomOrder.hpp are

        #pragma omp parallel for num_threads(nthreads)
        for (size_t b = 0; b < nobs.size(); ++b) {

and sincesize_t is unsigned long long, the second line won't compile. For Windows, the second line should be something like for (long long b = 0; b < static_cast<long long>(nobs.size()); ++b) { This problem occurs in various places throughout the code.

LTLA commented 1 year ago

Hm. Well, this is going to be a problem, because I use size_t in OpenMP'd loops a lot.

AFAICT this is because VS only supports an ancient version of OpenMP (2.0), whereas unsigned indices are allowed in 3.0+. Perhaps using openmp:llvm might help?

GregReese219 commented 1 year ago

On Tue, Jan 17, 2023 at 10:31 AM Aaron Lun @.***> wrote:

Hm. Well, this is going to be a problem, because I use size_t in OpenMP'd loops a lot.

Yeah, I started to make the change but then saw how ubiquitous it would be. I'll look into the openmp:llvm option you suggest below.

AFAICT this is because VS only supports an ancient version of OpenMP (2.0), whereas unsigned indices are allowed in 3.0+. Perhaps using openmp:llvm https://devblogs.microsoft.com/cppblog/improved-openmp-support-for-cpp-in-visual-studio/#new-openmpllvm-switch might help?

— Reply to this email directly, view it on GitHub https://github.com/LTLA/CppMnnCorrect/issues/10#issuecomment-1385854952, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5DENZV3NXIRVPJNOPLJEQ3WS3QP5ANCNFSM6AAAAAAT6EEPU4 . You are receiving this because you authored the thread.Message ID: @.***>

LTLA commented 1 year ago

FWIW you can override the parallelization mechanism by setting the MNNCORRECT_CUSTOM_PARALLEL macro:

https://github.com/jkanche/scran.js/blob/master/src/parallel.h

In the linked example, I switch to std::thread instead of OpenMP, as WebAssembly doesn't support OpenMP. So you could also use that approach if you want to use std::thread, or an OpenMP loop with signed indices.

GregReese219 commented 1 year ago

Aaron,

Using openmp:llvm did the trick. Here are the steps I used, which you might want to include in your readme file to help your Windows users:

For Visual Studio 2019

  1. Go to project properties -> configuration properties -> C/C++ -> Language and set Open MP Support to Yes (/openmp)
  2. Go to project properties -> configuration properties -> C/C++ -> Command Line and in the Additional Options box, add -openmp:llvm

Greg

P.S. - Next step is to get MnnCorrect running on the Mac

On Tue, Jan 17, 2023 at 10:31 AM Aaron Lun @.***> wrote:

Hm. Well, this is going to be a problem, because I use size_t in OpenMP'd loops a lot.

AFAICT this is because VS only supports an ancient version of OpenMP (2.0), whereas unsigned indices are allowed in 3.0+. Perhaps using openmp:llvm https://devblogs.microsoft.com/cppblog/improved-openmp-support-for-cpp-in-visual-studio/#new-openmpllvm-switch might help?

— Reply to this email directly, view it on GitHub https://github.com/LTLA/CppMnnCorrect/issues/10#issuecomment-1385854952, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5DENZV3NXIRVPJNOPLJEQ3WS3QP5ANCNFSM6AAAAAAT6EEPU4 . You are receiving this because you authored the thread.Message ID: @.***>

GregReese219 commented 4 months ago

Hi Aaron,

I thought I'd update you that the FlowJo MNN plugin https://www.flowjo.com/exchange/#/plugin/profile?id=65 now has over 350 downloads. Feel free to let your co-authors know.

On a different note, BD (Becton Dickinson) just had another round of layoffs and unfortunately I was one of the people who got zapped. I'm looking for a scientific or engineering coding or algorithms job in C++. You can see my background on my LinkedIn profile https://www.linkedin.com/in/gregreese219/. If you happen to run across something you think I'd be interested in, I'd appreciate hearing about it.

Take care.

Greg

On Wed, Jan 18, 2023 at 4:11 PM Aaron Lun @.***> wrote:

FWIW you can override the parallelization mechanism by setting the MNNCORRECT_CUSTOM_PARALLEL macro:

https://github.com/jkanche/scran.js/blob/master/src/parallel.h

In the linked example, I switch to std::thread instead of OpenMP, as WebAssembly doesn't support OpenMP. So you could also use that approach if you want to use std::thread, or an OpenMP loop with signed indices.

— Reply to this email directly, view it on GitHub https://github.com/LTLA/CppMnnCorrect/issues/10#issuecomment-1396264488, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5DENZUVB3WTUJJG2MFGZ5DWTCBBZANCNFSM6AAAAAAT6EEPU4 . You are receiving this because you authored the thread.Message ID: @.***>

LTLA commented 4 months ago

Oof. That's rough. Hope you got a decent severance package.

I'll pass along your name along; I've heard rumors of some internal C++ projects that might fit your description. But don't get your hopes up, looks like headcount's tight everywhere these days.

GregReese219 commented 4 months ago

Thanks man

On Sun, May 12, 2024 at 4:44 PM Aaron Lun @.***> wrote:

Oof. That's rough. Hope you got a decent severance package.

I'll pass along your name along; I've heard rumors of some internal C++ projects that might fit your description. But don't get your hopes up, looks like headcount's tight everywhere these days.

— Reply to this email directly, view it on GitHub https://github.com/LTLA/CppMnnCorrect/issues/10#issuecomment-2106413494, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5DENZQME22WJ5XOJAIK3UDZB75HVAVCNFSM6AAAAAAT6EEPU6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGQYTGNBZGQ . You are receiving this because you authored the thread.Message ID: @.***>