windows platform support #114
Conversation
| N=2048, | ||
| do_smooth=False, | ||
| ): | ||
| - preset_name = str(folder).split("/")[-2] |
There was a problem hiding this comment.
@mazeyu This path manipulation should be done with pathlib Path().parts[-2], which is multiplatform and would avoid the need for a windows only .patch file
| if (auxs == NULL) n_auxiliaries = 0; | ||
| #pragma omp parallel for | ||
| - for (size_t idx = 0; idx < size; idx++) { | ||
| + for (int idx = 0; idx < size; idx++) { |
There was a problem hiding this comment.
This type change should be made to be a single type that is multiplatform. Is size_t not supported on windows? If so we should use a different type, but it should be some other unsigned int type rather than a signed int.
There was a problem hiding this comment.
Yes, I received error C3016: 'idx': the index variable in the OpenMP 'for' statement must be a signed integer when compiling with MSVC 19.
Can we replace this type with a macro and let CMake determine this type for different platforms?
| \ No newline at end of file | ||
| + if sys.platform == 'win32': | ||
| + #replace suffix .so to .dll | ||
| + path = Path(path) |
There was a problem hiding this comment.
this can be included into the main repo rather than a .patch file
| @@ -0,0 +1,97 @@ | |||
| cmake_minimum_required(VERSION 3.2) | |||
There was a problem hiding this comment.
These seem great and very useful even independently of windows, @mazeyu please review
|
We appreciate your PR - I have sent it off to @mazeyu to test aswell. Is there any chance you could divide up the changes into more commits (especially a separate commit for the Cmake stuff) as this would make it easier to accept / include only part of the PR if we deem that necessary. Also, I likely will not accept any PR that uses a patch file, as I feel this is hard to maintain. We will work towards a single set of code that works on all platforms, but dont feel obliged to make these changes yourself as hopefully Zeyu the code owner can look at them once you have split up the rest of your changes into separate commits. |
Thank you. I submitted that the patch file was simply an attempt to provide testing for multi-platform support without disrupting the existing code. I will separate it and resubmit the changes. |
|
All makes sense, just a formatting change really, and still a huge huge benefit to everyone as it is, we really appreciate the work. The multiple commits thing also isn't strictly necessary as I think we will likely merge everything here. If you did have time to move things from the patch into the source files that would be awesome but no obligation as this is already great work. |
add patch and cmake file
tested on Windows10 11th Gen Intel(R) Core(TM) i7-11700 NVIDIA GeForce RTX 3070