Allow to set space group origin in PDB CRYST1 section

[afonari]

First part is to always parse space group origin:

• This is fixed by removed overloaded version of SpaceGroup::SetHMName.
• Unrelated, removed similar overloaded SpaceGroup::SetHallName.

Second part:

• Expose -xo option for write PDB, to set space group setting (if present) in space group label.

Added tests.

[afonari]

Pinging @jbrefort, it seems he added the code that I'm trying to remove.

[afonari]

NVM, after looking closely, I think all the functionality of that commit is preserved, so good for review.

[afonari]

New tests run and pass:

        Start  34: test_cifspacegroup_5
 33/164 Test  #34: test_cifspacegroup_5 .............   Passed    0.63 sec
        Start  35: test_cifspacegroup_6
 34/164 Test  #35: test_cifspacegroup_6 .............   Passed    0.70 sec

[afonari]

Added tests.

[afonari]

I think this is ready for review, I included modified space-groups.txt from #1560 in here for testing purposes (it could be merged from here to close #1560 also).

[afonari]

I've been testing this version for loading CIF files and converting them to PDB, seems to work well.

[afonari]

Can anyone review and merge this please?

[baoilleach]

Hi Alexandr, I've been a bit confused by all of the commits.

Getting pull requests is great, but the usual development process is to push to your own branch and only once it's ready to submit a pull request. Otherwise all of the developers get emails for every commit, and don't know when it's finished.

Anyway, back to the topic on hand. I'm not really familiar with the crystal code. Can you provide a short description of what problem or feature this pull request solves/adds and how it does it

[afonari]

Hi Noel,

Otherwise all of the developers get emails for every commit, and don't know when it's finished.

I apologize for that. It was a combination of self-review and improvements.

Can you provide a short description of what problem or feature this pull request solves/adds and how it does it

1. Align space-groups.txt with spglib: in particular (a) ensure that each of the Hall names present in the spglib is present (only once) in the space-groups.txt; (b) Ensure that HM names are group (aliased) to the correct Hall name (this solves issue HM name is the same for several definitions of space groups #1559); (c) ensure that space operators are correct for each Hall name (this fixes space groups that weren't fixed in Fixed a few errors in space-groups.txt #367) compared to spglib.

2. Remove overloaded SpaceGroup::SetHMName and SpaceGroup::SetHallName that were expecting const char *name. Now the only signature is const std::string &name for both functions. This is needed to always correctly parse origin by calling SpaceGroup::SetHMName(const std::string &name). Added code to correctly deal with :H origin, needed for rhombohedral groups.

3. Expose -xo option for PDB writer, to set space group setting (if present) in space group label.

[baoilleach]

No worries.

Sounds good, except for point 2. Functions should always take const char* instead of std::string (or reference to). I plan to remove all duplicate functions in the near future. I'm not sure from your description if you are saying you need the std::string version for some reason...? Feel free to point me to the code.

[afonari]

There were two overloaded variants of SetHallName (spacegroup.h):

void SetHallName(const std::string &name)
          { m_Hall = name; }

and (removed in this pull request):

void SetHallName(const char *name)
          { m_Hall = name; }

For SetHMName:

  void SpaceGroup::SetHMName(const std::string &name)
  {
    std::string::size_type idx = name.find(':');
    if (idx != std::string::npos)
      {
        std::string origin = name.substr(idx + 1, std::string::npos);
        if (origin == "H")
          {
            m_OriginAlternative = HEXAGONAL_ORIGIN;
          } else {
            m_OriginAlternative = atoi (origin.c_str());
          }
      }
    m_HM = name;
  }

and (removed in this pull request):

void SetHMName(const char *name)
          { m_HM = name; }

In the SetHMName(const std::string &name) variant, origin was parsed and set, whereas in SetHMName(const char *name) it wasn't.
I removed SetHMName(const char *name) and SetHallName(const char *name) for consistency.

Functions should always take const char* instead of std::string (or reference to).

I don't have any issues removing either of the signatures, as long as parsing origin is kept.

[baoilleach]

A nice example of why we should avoid duplicate functions. Could you change the signature to take const char and then it's good to merge.

[afonari]

All done, related tests run and pass:

        Start  30: test_cifspacegroup_1
 29/168 Test  #30: test_cifspacegroup_1 .............   Passed    0.70 sec
        Start  31: test_cifspacegroup_2
 30/168 Test  #31: test_cifspacegroup_2 .............   Passed    0.61 sec
        Start  32: test_cifspacegroup_3
 31/168 Test  #32: test_cifspacegroup_3 .............   Passed    0.10 sec
        Start  33: test_cifspacegroup_4
 32/168 Test  #33: test_cifspacegroup_4 .............   Passed    0.62 sec
        Start  34: test_cifspacegroup_5
 33/168 Test  #34: test_cifspacegroup_5 .............   Passed    0.61 sec
        Start  35: test_cifspacegroup_6
 34/168 Test  #35: test_cifspacegroup_6 .............   Passed    0.61 sec
        Start  36: test_cifspacegroup_7
 35/168 Test  #36: test_cifspacegroup_7 .............   Passed    0.61 sec
        Start  37: test_cifspacegroup_8
 36/168 Test  #37: test_cifspacegroup_8 .............   Passed    0.66 sec
        Start  38: test_cifspacegroup_9
 37/168 Test  #38: test_cifspacegroup_9 .............   Passed    0.61 sec
        Start  39: test_cifspacegroup_10
 38/168 Test  #39: test_cifspacegroup_10 ............   Passed    0.61 sec

[baoilleach]

Thanks.