map name does not name a type error

[LMHmedchem]

Hello,

I wrote the beginnings of a very simple map to use as a lookup.

#include <string>
#include <map>

using namespace std;

// declare map
map<string, unsigned int> electron_lookup;

// add some values
electron_lookup["C"] = 6;
electron_lookup["N"] = 7;
electron_lookup["O"] = 8;

I am getting a compiler error,

error: ‘electron_lookup’ does not name a type

from the lines where the map key/value pairs are created.

I have also tried the more verbose,

electron_lookup.insert(make_pair("C", 6));
electron_lookup.insert(make_pair("N", 7));
electron_lookup.insert(make_pair("O", 8));

but get the same error. This map is declared as a global, meaning it resides outside curly braces at the beginning of the src file where it is used. I am not sure this is the best place for it. This should get populated once and then will be accessed many times, probably from more than one place in the code. For that reason, it should be const (it will never change), but I need to get it working.

The code seems to compile fine if I move the definition into a {} block. Do I need some kind of loading function? That seems excessive for an ultra simple const lookup table.

I don't seem to remember ever having problems with a simple map before.

This is g++ (GCC) 5.3.0 under cygwin.

Thanks,

LMHmedchem

[wolle]

This map is declared as a global,

I'm pretty certain the global declaration has something to do with it. You can check that hypothesis by declaring it local and see if the problem goes away.

If I wanted a global map I would implement it as a Singleton. It's not good to have global data but since this map is to be used as a constant it would be fine.

I would have a .h file like this. It would be included anywhere the constants are needed. I changed from map to unordered_map because it has faster accesses (complexity drops from O(log N) to O(1)).

#ifndef MY_CONSTANTS_ONCE // include guard
#define MY_CONSTANTS_ONCE

#include <string>
#include <unordered_map>

namespace my_constants {

	using Electron_lookup = std::unordered_map<std::string, unsigned int>; // the electron lookup type
                                                               
	inline const Electron_lookup& electron_lookup() { // a singleton
		static const Electron_lookup table = {
			{"C", 6},
			{"N", 7},
			{"O", 8},
		};
		return table;
	}

}

#endif

It can be used like this,

void test() {
	std::cout << my_constants::electron_lookup().at("C") << std::endl;
}

[Richard.J]

Most likely, you cannot add elements to the map outside any function, i.e. in the global scope.
However, something like this should work:

map<string, unsigned int> electron_lookup{
    {"C", 6},
    {"N", 7},
    {"O", 8},
};

[wolle]

Well after I had realized that my Singleton approach was unnecessarily complicated I would've changed to an ordinary global function instead like,

#ifndef MY_CONSTANTS_ONCE
#define MY_CONSTANTS_ONCE

#include <string>
#include <unordered_map>


namespace my_constants {

	inline unsigned int electron_lookup(const std::string& s) { // global function
		static std::unordered_map<std::string, unsigned int> table = {
			{"C", 6},
			{"N", 7},
			{"O", 8},
		};
		return table[s];
	}

}

#endif

//

void test() {
	std::cout << my_constants::electron_lookup("C") << std::endl;
}

[LMHmedchem]

Well after I had realized that my Singleton approach was unnecessarily complicated I would've changed to an ordinary global function instead like,

#ifndef MY_CONSTANTS_ONCE
#define MY_CONSTANTS_ONCE

#include <string>
#include <unordered_map>


namespace my_constants {

    inline unsigned int electron_lookup(const std::string& s) { // global function
        static std::unordered_map<std::string, unsigned int> table = {
            {"C", 6},
            {"N", 7},
            {"O", 8},
        };
        return table[s];
    }

}

#endif

//

void test() {
    std::cout << my_constants::electron_lookup("C") << std::endl;
}

I already have a constants.h file, so I added the lookup function there,

// constants.h

// include guard
#ifndef CONSTANTS_DEF
#define CONSTANTS_DEF

#include <string>
#include <map>

namespace local_const {

   // global function to access map to lookup number of electrons by element
   unsigned int electron_lookup(const std::string &lookup_element) {

      // declare map
      static std::map<std::string, unsigned int> electron_table;

      // add elements
      electron_table["H"] = 1;
      electron_table["C"] = 6;
      electron_table["N"] = 7;
      electron_table["O"] = 8;
      electron_table["F"] = 9;
      electron_table["P"] = 15;
      electron_table["S"] = 16;
      electron_table["Cl"] = 3;
      electron_table["Br"] = 35;
      electron_table["I"] = 53;

   return table[lookup_element];
   }

}

// end include guard
#endif

I am not using C++11 at this point so I have modified the code to make it compile. There doesn't seem to be an easy way to declare the map as const, though intuitively it seems as if it should be const.

Not that it will matter here as far as performance goes, but one of the reasons I was thinking about a global declaration is that this map will be accessed 10^5 times and more per file processed. Optimally, it should be populated at runtime and then just sit there waiting to be accessed. It seems to me that if it were declared in a function that it would be going in and out of scope every time the function is called and returns. That sounds like it would be coming on and off the stack all the time and doesn't sound very efficient.

Possibly the compiler is smarter than that and I just don't understand the resource allocation associated with the code. If someone could elaborate I would appreciate knowing more.

Thanks,

LMHmedchem

[wolle]

I am not using C++11 at this point so I have modified the code to make it compile.

Unfortunately I've no experience with C++98.

The only quick solution I can think of without too many changes is to only assign data when the map is empty. This will be the case once only. The check of whether the map is empty happens each time the function is called but it is very fast. The search of the map will most likely dominate the time spent in the function. Say there are 100 elements stored in the map then there will be something like 10 accesses inside the map on average to find the element(because it's based on a tree with logarithmic access)

Best would be to use an unordered_map because there will always be just 1.1 internal accesses or so on average but unfortunately unordered_map is not available in C++98. It could be worth it to use one from Boost! If that's not possible one improvement could be to pre-check elements that are frequent. In a biological application I suppose "H" is dominant. Therefore a special check could be put first before the map is searched.

Note that in C++11 a global function like this should be declared inline. I cannot tell for sure whether this also applies to C++98 but I suspect it does. It should be investigated further.

Also note that if an unordered_map equivalent from Boost is used no pre-checking should be done.

  inline unsigned int electron_lookup(const std::string &lookup_element) {

      // declare map
      static std::map<std::string, unsigned int> electron_table;

      // optimization: pre-check hydrogen
      if (lookup_element.compare("H") != 0) return 1;

      // add elements if map is empty, happens once only
      if (electron_table.empty()) { 
 //         electron_table["H"] = 1; // pre-checked
          electron_table["C"] = 6;
          electron_table["N"] = 7;
          electron_table["O"] = 8;
          electron_table["F"] = 9;
          electron_table["P"] = 15;
          electron_table["S"] = 16;
          electron_table["Cl"] = 3;
          electron_table["Br"] = 35;
          electron_table["I"] = 53;
       }
       return table[lookup_element];
   }

[2kaud]

Can Cl and Br be shortened to a single unique char?

[LMHmedchem]

Unfortunately I've no experience with C++98.

The only quick solution I can think of without too many changes is to only assign data when the map is empty. This will be the case once only. The check of whether the map is empty happens each time the function is called but it is very fast. The search of the map will most likely dominate the time spent in the function. Say there are 100 elements stored in the map then there will be something like 10 accesses inside the map on average to find the element(because it's based on a tree with logarithmic access)

Best would be to use an unordered_map because there will always be just 1.1 internal accesses or so on average but unfortunately unordered_map is not available in C++98. It could be worth it to use one from Boost! If that's not possible one improvement could be to pre-check elements that are frequent. In a biological application I suppose "H" is dominant. Therefore a special check could be put first before the map is searched.

Note that in C++11 a global function like this should be declared inline. I cannot tell for sure whether this also applies to C++98 but I suspect it does. It should be investigated further.

Also note that if an unordered_map equivalent from Boost is used no pre-checking should be done.

  inline unsigned int electron_lookup(const std::string &lookup_element) {

      // declare map
      static std::map<std::string, unsigned int> electron_table;

      // optimization: pre-check hydrogen
      if (lookup_element.compare("H") != 0) return 1;

      // add elements if map is empty, happens once only
      if (electron_table.empty()) { 
 //         electron_table["H"] = 1; // pre-checked
          electron_table["C"] = 6;
          electron_table["N"] = 7;
          electron_table["O"] = 8;
          electron_table["F"] = 9;
          electron_table["P"] = 15;
          electron_table["S"] = 16;
          electron_table["Cl"] = 3;
          electron_table["Br"] = 35;
          electron_table["I"] = 53;
       }
       return table[lookup_element];
   }

The code I posted compiles, I haven't checked it at runtime yet. For now, the map will only contain the 10 entries I listed above. It may actually be less overhead to just use an if else conditional in the function that is in the order of element frequency (H first, then C, etc).

inline unsigned int electron_lookup(const std::string &lookup_element) {

   // return electron count based on the passed element value
   if(lookup_element == "H") { return 1; }
   else if(lookup_element == "C") { return 6; }
   else if(lookup_element == "N") { return 7; }
   else if(lookup_element == "O") { return 8; }
   else if(lookup_element == "P") { return 15; }
   else if(lookup_element == "S") { return 16; }
   else if(lookup_element == "Cl") { return 17; }
   else if(lookup_element == "F") { return 9; }
   else if(lookup_element == "Br") { return 35; }
   else if(lookup_element == "I") { return 53; }
   else { cerr << lookup_element << "is not in the lookup table" << endl; exit(-1); }

}

It really seems as if it would be trivial to just declare and assign the elements of a data structure as a global variable and have it available in global scope for whatever part of the code needs it. We used to do this kind of thing all the time in F77, except those were just arrays of primitives.

The inline statement works with the above function but doesn't seem to compile with the map. I'm not sure why.

Can Cl and Br be shortened to a single unique char?

Unfortunately, elements can have two characters. I could convert the element symbol to an int or something like that, but I don't see how that would help.

LMHmedchem

[2kaud]

If the element symbol is convertable to an int, then you have a simple c-style array indexed on the int that returns the required value. This will potentially use memory that isn't actually used - but it will be quick. Consider for values "A", "B", "C" - 14, 15, 16 as an example:

const int values[26] = {14, 15, 16};
...
res = values[val - 'A'];

If there was no 'D' but an 'E' then you extend the array initialisation to be {14, 15, 16, 0, 17} etc etc. This could generate a potentially long array initialisation list. If you could upgrade to c++17 and use constexpr, then this would be done at compile time and not run-time.