thaler-lab / Wasserstein

Python/C++ library for computing Wasserstein distances efficiently.
https://thaler-lab.github.io/Wasserstein
Other
22 stars 9 forks source link

-1 value for enum types causing error #3

Closed lee218llnl closed 1 year ago

lee218llnl commented 1 year ago

I got a few compiler errors of the form:

./wasserstein/internal/EMDUtils.hh:135:14: error: enumerator value -1 is outside the range of underlying type 'char'

I believe enum types are supposed to be non-negative integers. I can work around this by setting the value to 2 instead of -1, but am not sure if that is the proper thing to do. Here are the 2 changes I made to get wasserstein to compile:

$ git diff wasserstein/internal/EMDUtils.hh wasserstein/internal/NetworkSimplex.hh
diff --git a/wasserstein/internal/EMDUtils.hh b/wasserstein/internal/EMDUtils.hh
index a8c5643..be76fd2 100644
--- a/wasserstein/internal/EMDUtils.hh
+++ b/wasserstein/internal/EMDUtils.hh
@@ -132,7 +132,7 @@ enum class EMDStatus : char {
 };

 enum class ExtraParticle : char {
-  Neither = -1,
+  Neither = 2,
   Zero = 0,
   One = 1
 };
diff --git a/wasserstein/internal/NetworkSimplex.hh b/wasserstein/internal/NetworkSimplex.hh
index 47af0f1..7b1acfc 100644
--- a/wasserstein/internal/NetworkSimplex.hh
+++ b/wasserstein/internal/NetworkSimplex.hh
@@ -294,7 +294,7 @@ private:

   // State constants for arcs
   enum ArcState : char {
-    STATE_UPPER = -1,
+    STATE_UPPER = 2,
     STATE_TREE  =  0,
     STATE_LOWER =  1
   };
100rab-S commented 1 year ago

@lee218llnl Yeah facing the same issue on an arm64 arch machine! The solution seems straight forward, but would be great if the author verifies it and update the code. @pkomiske Request you to please look into the issue. With this change, the library can be easily extended to various other machines.

pkomiske commented 1 year ago

Thanks for pointing this out. Apparently char can be signed or unsigned depending on the compiler. To make as minimal a change as possible, can you try changing the underlying enum type to signed char and leaving the values as they were?

100rab-S commented 1 year ago

@pkomiske Yeah it did work by specifying the type of char on arm64 machine. So shall I raise a PR with the changes for fixing this bug in the master? Apparently you can test the changes and release the updated version on PyPI!