added none or 0 to unset bad channels (#632)

* added none or 0 to unset bad channels

* free function split to get int array from string of arguments for badchannels

* missed a file

* allowing list for badchannels in command line

* added badchannels in python

* added size check

* more comments in Detector.h and added more tests for facny command line badchannels

* removeDuplicates accept any container, added tests

* corner cases: 1:5,6,7 or 5,6,7 or just 1:5

Co-authored-by: Erik Frojdh <erik.frojdh@gmail.com>

python/slsdet/detector.py

@@ -1604,6 +1604,19 @@ class Detector(CppDetectorApi):
     def sync(self, value):
         ut.set_using_dict(self.setSynchronization, value)
 
+    @property
+    @element
+    def badchannels(self):
+        """
+        [fname|none|0]\n\t[Gotthard2][Mythen3] Sets the bad channels (from file of bad channel numbers) to be masked out. None or 0 unsets all the badchannels.\n
+        [Mythen3] Also does trimming
+        """
+        return self.getBadChannels()
+
+    @badchannels.setter
+    def badchannels(self, value):
+        ut.set_using_dict(self.setBadChannels, value)
+
     @property
     @element
     def lock(self):

python/src/detector.cpp

@@ -219,6 +219,21 @@ void init_det(py::module &m) {
         (void (Detector::*)(const std::string &, sls::Positions)) &
             Detector::setBadChannels,
         py::arg(), py::arg() = Positions{});
+    CppDetectorApi.def(
+        "getBadChannels",
+        (Result<std::vector<int>>(Detector::*)(sls::Positions) const) &
+            Detector::getBadChannels,
+        py::arg() = Positions{});
+    CppDetectorApi.def(
+        "setBadChannels",
+        (void (Detector::*)(const std::vector<int>, sls::Positions)) &
+            Detector::setBadChannels,
+        py::arg(), py::arg() = Positions{});
+    CppDetectorApi.def(
+        "setBadChannels",
+        (void (Detector::*)(const std::vector<std::vector<int>>)) &
+            Detector::setBadChannels,
+        py::arg());
     CppDetectorApi.def("isVirtualDetectorServer",
                        (Result<bool>(Detector::*)(sls::Positions) const) &
                            Detector::isVirtualDetectorServer,

slsDetectorSoftware/include/sls/Detector.h

@@ -221,6 +221,17 @@ class Detector {
      */
     void setBadChannels(const std::string &fname, Positions pos = {});
 
+    /** [Gotthard2][Mythen3] */
+    Result<std::vector<int>> getBadChannels(Positions pos = {}) const;
+
+    /** [Gotthard2][Mythen3] Empty list resets bad channel list */
+    void setBadChannels(const std::vector<int> list, Positions pos = {});
+
+    /** [Gotthard2][Mythen3] Size of list should match number of modules. Each
+     * value is at module level and can start at 0. Empty vector resets bad
+     * channel list. */
+    void setBadChannels(const std::vector<std::vector<int>> list);
+
     Result<bool> isVirtualDetectorServer(Positions pos = {}) const;
     ///@}
 

slsDetectorSoftware/src/CmdProxy.cpp

@@ -7,6 +7,7 @@
 #include "sls/ToString.h"
 #include "sls/bit_utils.h"
 #include "sls/container_utils.h"
+#include "sls/file_utils.h"
 #include "sls/logger.h"
 #include "sls/sls_detector_defs.h"
 
@@ -548,9 +549,9 @@ std::string CmdProxy::BadChannels(int action) {
     std::ostringstream os;
     os << cmd << ' ';
     if (action == defs::HELP_ACTION) {
-        os << "[fname]\n\t[Gotthard2][Mythen3] Sets the bad channels (from "
+        os << "[fname|none|0]\n\t[Gotthard2][Mythen3] Sets the bad channels "
-              "file of bad channel numbers) to be masked out."
+              "(from file of bad channel numbers) to be masked out. None or 0 "
-              "\n\t[Mythen3] Also does trimming"
+              "unsets all the badchannels.\n\t[Mythen3] Also does trimming"
            << '\n';
     } else if (action == defs::GET_ACTION) {
         if (args.size() != 1) {
@@ -559,10 +560,25 @@ std::string CmdProxy::BadChannels(int action) {
         det->getBadChannels(args[0], std::vector<int>{det_id});
         os << "successfully retrieved" << '\n';
     } else if (action == defs::PUT_ACTION) {
-        if (args.size() != 1) {
+        bool parse = false;
+        if (args.size() == 0) {
             WrongNumberOfParameters(1);
+        } else if (args.size() == 1) {
+            if (args[0] == "none" || args[0] == "0") {
+                det->setBadChannels(std::vector<int>{},
+                                    std::vector<int>{det_id});
+            } else if (args[0].find(".") != std::string::npos) {
+                det->setBadChannels(args[0], std::vector<int>{det_id});
+            } else {
+                parse = true;
+            }
+        }
+        // parse multi args or single one with range or single value
+        if (parse || args.size() > 1) {
+            // get channels
+            auto list = getChannelsFromStringList(args);
+            det->setBadChannels(list, std::vector<int>{det_id});
         }
-        det->setBadChannels(args[0], std::vector<int>{det_id});
         os << "successfully loaded" << '\n';
     } else {
         throw RuntimeError("Unknown action");

slsDetectorSoftware/src/Detector.cpp

@@ -349,6 +349,29 @@ void Detector::setBadChannels(const std::string &fname, Positions pos) {
     pimpl->setBadChannels(fname, pos);
 }
 
+Result<std::vector<int>> Detector::getBadChannels(Positions pos) const {
+    return pimpl->Parallel(&Module::getBadChannels, pos);
+}
+
+void Detector::setBadChannels(const std::vector<std::vector<int>> list) {
+
+    if (list.size() != static_cast<size_t>(size())) {
+        std::stringstream ss;
+        ss << "Number of bad channel sets (" << list.size()
+           << ") needs to match the number of modules (" << size() << ")";
+        throw RuntimeError(ss.str());
+    }
+
+    for (int idet = 0; idet < size(); ++idet) {
+        // TODO! Call in parallel since loading trimbits is slow?
+        pimpl->Parallel(&Module::setBadChannels, {idet}, list[idet]);
+    }
+}
+
+void Detector::setBadChannels(const std::vector<int> list, Positions pos) {
+    pimpl->setBadChannels(list, pos);
+}
+
 Result<bool> Detector::isVirtualDetectorServer(Positions pos) const {
     return pimpl->Parallel(&Module::isVirtualDetectorServer, pos);
 }

slsDetectorSoftware/src/DetectorImpl.cpp

@@ -1778,6 +1778,10 @@ void DetectorImpl::setBadChannels(const std::string &fname, Positions pos) {
     if (list.empty()) {
         throw RuntimeError("Bad channel file is empty.");
     }
+    setBadChannels(list, pos);
+}
+
+void DetectorImpl::setBadChannels(const std::vector<int> list, Positions pos) {
 
     // update to multi values if multi modules
     if (isAllPositions(pos)) {

slsDetectorSoftware/src/DetectorImpl.h

@@ -84,7 +84,9 @@ class DetectorImpl : public virtual slsDetectorDefs {
      */
     virtual ~DetectorImpl();
 
-    template <class CT> struct NonDeduced { using type = CT; };
+    template <class CT> struct NonDeduced {
+        using type = CT;
+    };
     template <typename RT, typename... CT>
     Result<RT> Parallel(RT (Module::*somefunc)(CT...),
                         std::vector<int> positions,
@@ -306,6 +308,7 @@ class DetectorImpl : public virtual slsDetectorDefs {
 
     void getBadChannels(const std::string &fname, Positions pos) const;
     void setBadChannels(const std::string &fname, Positions pos);
+    void setBadChannels(const std::vector<int> list, Positions pos);
 
     std::vector<std::string> getCtbDacNames() const;
     std::string getCtbDacName(defs::dacIndex i) const;

slsDetectorSoftware/tests/test-CmdProxy.cpp

@@ -644,6 +644,8 @@ TEST_CASE("badchannels", "[.cmd]") {
     auto det_type = det.getDetectorType().squash();
 
     if (det_type == defs::GOTTHARD2 || det_type == defs::MYTHEN3) {
+        auto prev = det.getBadChannels();
+
         REQUIRE_THROWS(proxy.Call("badchannels", {}, -1, GET));
 
         std::string fname_put =
@@ -656,6 +658,51 @@ TEST_CASE("badchannels", "[.cmd]") {
         std::vector<int> expected = {0, 12, 15, 40, 41, 42, 43, 44, 1279};
         REQUIRE(list == expected);
 
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"none"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        REQUIRE(list.empty());
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_put}, 0, PUT));
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"0"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        REQUIRE(list.empty());
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"12"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        expected = {12};
+        REQUIRE(list == expected);
+
+        REQUIRE_NOTHROW(proxy.Call(
+            "badchannels", {"0", "12,", "15", "43", "40:45", "1279"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        expected = {0, 12, 15, 40, 41, 42, 43, 44, 1279};
+        REQUIRE(list == expected);
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"40:45"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        expected = {40, 41, 42, 43, 44};
+        REQUIRE(list == expected);
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"5,6,7"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        expected = {5, 6, 7};
+        REQUIRE(list == expected);
+
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {"1:5,6,7"}, 0, PUT));
+        REQUIRE_NOTHROW(proxy.Call("badchannels", {fname_get}, 0, GET));
+        list = getChannelsFromFile(fname_get);
+        expected = {1, 2, 3, 4, 6, 7};
+        REQUIRE(list == expected);
+
+        det.setBadChannels(prev);
+
     } else {
         REQUIRE_THROWS(proxy.Call("badchannels", {}, -1, GET));
     }

slsSupportLib/include/sls/container_utils.h

@@ -148,6 +148,18 @@ Squash(const Container &c, typename Container::value_type default_value = {}) {
     return default_value;
 }
 
+template <typename T>
+typename std::enable_if<is_container<T>::value, bool>::type
+removeDuplicates(T &c) {
+    auto containerSize = c.size();
+    std::sort(c.begin(), c.end());
+    c.erase(std::unique(c.begin(), c.end()), c.end());
+    if (c.size() != containerSize) {
+        return true;
+    }
+    return false;
+}
+
 } // namespace sls
 
 #endif // CONTAINER_UTILS_H

slsSupportLib/include/sls/file_utils.h

@@ -50,6 +50,8 @@ ssize_t getFileSize(FILE *fd, const std::string &prependErrorString);
 
 std::string getFileNameFromFilePath(const std::string &fpath);
 
+std::vector<int> getChannelsFromStringList(const std::vector<std::string> list);
+
 /** File can have # for comments.
  * Channels can be separated by spaces, commas
  * and ranges provided using ':', eg. 23:29

slsSupportLib/src/file_utils.cpp

@@ -2,6 +2,7 @@
 // Copyright (C) 2021 Contributors to the SLS Detector Package
 #include "sls/file_utils.h"
 #include "sls/ToString.h"
+#include "sls/container_utils.h"
 #include "sls/logger.h"
 #include "sls/sls_detector_exceptions.h"
 
@@ -165,6 +166,63 @@ ssize_t getFileSize(FILE *fd, const std::string &prependErrorString) {
     return fileSize;
 }
 
+std::vector<int>
+getChannelsFromStringList(const std::vector<std::string> list) {
+    std::vector<int> channels;
+    for (auto line : list) {
+
+        // replace comma with space
+        std::replace_if(
+            begin(line), end(line), [](char c) { return (c == ','); }, ' ');
+
+        // split line (delim space)
+        std::vector<std::string> vec = split(line, ' ');
+
+        // for every channel separated by space
+        for (auto it : vec) {
+            // find range and replace with sequence of x to y
+            auto result = it.find(':');
+            if (result != std::string::npos) {
+                try {
+                    int istart = StringTo<int>(it.substr(0, result));
+                    int istop = StringTo<int>(
+                        it.substr(result + 1, it.length() - result - 1));
+                    LOG(logDEBUG1) << "istart:" << istart << " istop:" << istop;
+                    std::vector<int> range(istop - istart);
+                    std::generate(range.begin(), range.end(),
+                                  [n = istart]() mutable { return n++; });
+                    for (auto range_it : range) {
+                        channels.push_back(range_it);
+                    }
+                } catch (std::exception &e) {
+                    throw RuntimeError(
+                        "Could not load channels. Invalid channel range: " +
+                        it);
+                }
+            }
+
+            // else convert to int
+            else {
+                int ival = 0;
+                try {
+                    ival = StringTo<int>(it);
+                } catch (std::exception &e) {
+                    throw RuntimeError(
+                        "Could not load channels. Invalid channel number: " +
+                        it);
+                }
+                channels.push_back(ival);
+            }
+        }
+    }
+
+    if (removeDuplicates(channels)) {
+        LOG(logWARNING) << "Removed duplicates from channel file";
+    }
+    LOG(logDEBUG1) << "list:" << ToString(channels);
+    return channels;
+}
+
 std::vector<int> getChannelsFromFile(const std::string &fname) {
     // read bad channels file
     std::ifstream input_file(fname);
@@ -172,73 +230,19 @@ std::vector<int> getChannelsFromFile(const std::string &fname) {
         throw RuntimeError("Could not open bad channels file " + fname +
                            " for reading");
     }
-    std::vector<int> list;
+    std::vector<std::string> lines;
     for (std::string line; std::getline(input_file, line);) {
         // ignore comments
         if (line.find('#') != std::string::npos) {
             line.erase(line.find('#'));
         }
-
+        // ignore empty lines
-        // replace comma with space
+        if (line.empty()) {
-        std::replace_if(
+            continue;
-            begin(line), end(line), [](char c) { return (c == ','); }, ' ');
-
-        // replace x:y with a sequence of x to y
-        auto result = line.find(':');
-        while (result != std::string::npos) {
-            auto start = line.rfind(' ', result);
-            if (start == std::string::npos) {
-                start = 0;
-            } else
-                ++start;
-            int istart = StringTo<int>(line.substr(start, result - start));
-
-            auto stop = line.find(' ', result);
-            if (stop == std::string::npos) {
-                stop = line.length();
-            }
-            int istop =
-                StringTo<int>(line.substr(result + 1, stop - result - 1));
-
-            std::vector<int> v(istop - istart);
-            std::generate(v.begin(), v.end(),
-                          [n = istart]() mutable { return n++; });
-            line.replace(start, stop - start, ToString(v));
-
-            LOG(logDEBUG1) << line;
-            result = line.find(':');
-        }
-
-        // remove punctuations including [ and ]
-        line.erase(std::remove_if(begin(line), end(line), ispunct), end(line));
-
-        LOG(logDEBUG) << "\nline: [" << line << ']';
-
-        // split line (delim space) and push to list
-        std::vector<std::string> vec = split(line, ' ');
-        for (auto it : vec) {
-            int ival = 0;
-            try {
-                ival = StringTo<int>(it);
-            } catch (std::exception &e) {
-                throw RuntimeError("Could not load channels from file. Invalid "
-                                   "channel number: " +
-                                   it);
-            }
-            list.push_back(ival);
         }
+        lines.push_back(line);
     }
-
+    return getChannelsFromStringList(lines);
-    // remove duplicates from list
-    auto listSize = list.size();
-    std::sort(list.begin(), list.end());
-    list.erase(unique(list.begin(), list.end()), list.end());
-    if (list.size() != listSize) {
-        LOG(logWARNING) << "Removed duplicates from channel file";
-    }
-
-    LOG(logDEBUG1) << "list:" << ToString(list);
-    return list;
 }
 
 std::string getAbsolutePathFromCurrentProcess(const std::string &fname) {

slsSupportLib/tests/test-container_utils.cpp

@@ -136,4 +136,19 @@ TEST_CASE("compare a vector of arrays", "[support]") {
     CHECK(minusOneIfDifferent(vec1) == arr);
 }
 
+TEST_CASE("remove duplicates from vector"){
+    std::vector<int> v{5,6,5,3};
+    auto r = removeDuplicates(v);
+    CHECK(r == true); //did indeed remove elements
+    CHECK( v == std::vector<int>{3,5,6});
+}
+
+TEST_CASE("remove duplicated empty vector"){
+    std::vector<int> v;
+    auto r = removeDuplicates(v);
+    CHECK(r == false); //no elements to remove
+    CHECK( v == std::vector<int>{});
+}
+
+
 } // namespace sls