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

Fix compile errors with msvc9 and msvc10 #129

Merged
merged 2 commits into from
Jun 21, 2016
Merged

Fix compile errors with msvc9 and msvc10 #129

merged 2 commits into from
Jun 21, 2016

Conversation

cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Jun 20, 2016

I took the definition of NAN from Cython.
See also #128

memset(&value, 0xFF, sizeof(value));
return value;
}
#define NAN __NAN()
Copy link
Contributor

Choose a reason for hiding this comment

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

@kwgoodman what if we changed line 14 to:

const double NAN = __NAN()
const double NAN64 = __NAN()
const float NAN32 = __NAN()

This will avoid calling memset() repeatedly, as there will be only one NAN value. Also, later down the line, we can turn move_median into a template so that it is fast for float32 as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jennolsen84, I had trouble redefining NAN using const double NAN = __NAN(). I ended up trying:

kg@kg:/devel/bottleneck (cgohlke-patch-1 *)$ di
diff --git a/bottleneck/csrc/move_median.c b/bottleneck/csrc/move_median.c
index cf94d83..ff5d477 100644
--- a/bottleneck/csrc/move_median.c
+++ b/bottleneck/csrc/move_median.c
@@ -10,6 +10,11 @@ node1->idx = idx2;                                 \
 node2->idx = idx1;                                 \
 idx1       = idx2

+/* define nan */
+ai_t myNAN;
+void init_nan(void) {
+  memset(&myNAN, 0xFF, sizeof(ai_t));
+}

 /*
 -----------------------------------------------------------------------------
@@ -64,6 +69,8 @@ mm_new(const idx_t window, idx_t min_count)

     mm_reset(mm);

+    init_nan();
+
     return mm;
 }

@@ -511,7 +518,7 @@ mm_get_median(mm_handle *mm)
 {
     idx_t n_total = mm->n_l + mm->n_s;
     if (n_total < mm->min_count)
-        return NAN;
+        return myNAN;
     if (min(mm->window, n_total) % 2 == 1)
         return mm->s_heap[0]->ai;
     return (mm->s_heap[0]->ai + mm->l_heap[0]->ai) / 2.0;
diff --git a/bottleneck/csrc/move_median.h b/bottleneck/csrc/move_median.h
index d6bbe54..736b0b0 100644
--- a/bottleneck/csrc/move_median.h
+++ b/bottleneck/csrc/move_median.h
@@ -6,12 +6,6 @@

 #if defined(_MSC_VER) && (_MSC_VER < 1900)
 #define inline __inline
-static __inline float __NAN() {
-  float value;
-  memset(&value, 0xFF, sizeof(value));
-  return value;
-}
-#define NAN __NAN()
 #endif

 #if NOCYTHON==1

Note that NAN is only used in one place---calculating the median when the count is less than min_count. These changes slow the code a bit, but I wasn't too careful in my measurement.

@kwgoodman kwgoodman mentioned this pull request Jun 21, 2016
@kwgoodman kwgoodman merged commit 69970f3 into pydata:master Jun 21, 2016
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