Added LinearSG species

[dmorse]

Josh - I knew there was a problem because the automatic tests failed. The automatic tests are run with angles, dihedrals and external potentials enabled (-a1 -d1 -e1) so make sure you test with same flags.

[jmysona]

Dave,

I made the changes as requested. The code has passed the tests performed by travis and restarts properly.

--Josh

On Sat, Feb 24, 2018 at 11:01 AM, David Morse notifications@github.com wrote:

Josh - I knew there was a problem because the automatic tests failed. The automatic tests are run with angles, dihedrals and external potentials enabled (-a1 -d1 -e1) so make sure you test with same flags.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dmorse/simpatico/pull/48#issuecomment-368242536, or mute the thread https://github.com/notifications/unsubscribe-auth/AIEb6oGdPyyZhnMZBMsVf_dlPoW7K-xGks5tYEB3gaJpZM4SRnp5 .

[dmorse]

Josh, I merged the pull request, because nothing appeared wrong, but didn't understand why you chose to implement this function this way:

void LinearSG::setMoleculeState(Molecule& molecule, int stateId) { int nAtom = molecule.nAtom(); DArray beadIdentities; for (int i = 0; i < nAtom; ++i) { if (stateId == 0) { beadIdentities = beadTypeIds0; } else { beadIdentities = beadTypeIds1; } molecule.atom(i).setTypeId(beadIdentities[i]); } setMoleculeStateId(molecule, stateId); }

Questions: 1) Why repeatedly make a copy of an entire array inside a loop over atoms, rather than copying the array before entering the loop? 2) Why make a temporary local copy of the entire appropriate beadTypeIds_ array at all? All you need to do is assign appropriate values to atom type Ids. Why not just:

   int typeId
   for (int i = 0; i < nAtom; ++i) {
        if (stateId == 0) {
            typeId = beadTypeIds0_[i];
        } else {
            typeId = beadTypeIds1_[i];
       }
       molecule.atom(i).setTypeId(beadTypeId);
  }

  If you agree, please change and send another tiny pull request. 

[jmysona]

That's a very good point. The repeated copying of the array is excessive. I think the original intention was to avoid having to evaluate an if statement multiple times.

Currently I've gone with a third option which avoids the repeated evaluation of the if statement; the code checks to see which subtype it should set the molecule to, then loops over the atoms assigning them their new types. The code is now

  int nAtom  = molecule.nAtom();
  if (stateId == 0) {
    for (int i = 0; i < nAtom; ++i) {
      molecule.atom(i).setTypeId(beadTypeIds0_[i]);
    }
  } else {
    for (int i = 0; i < nAtom; ++i) {
      molecule.atom(i).setTypeId(beadTypeIds1_[i]);
    }
  }
  setMoleculeStateId(molecule, stateId);

Let me know if this is acceptable

On Mon, Mar 5, 2018 at 8:55 PM, David Morse notifications@github.com wrote:

Josh, I merged the pull request, because nothing appeared wrong, but didn't understand why you chose to implement this function this way:

void LinearSG::setMoleculeState(Molecule& molecule, int stateId) { int nAtom = molecule.nAtom(); DArray beadIdentities; for (int i = 0; i < nAtom; ++i) { if (stateId == 0) { beadIdentities = beadTypeIds0; } else { beadIdentities = beadTypeIds1; } molecule.atom(i).setTypeId(beadIdentities[i]); } setMoleculeStateId(molecule, stateId); }

Questions:

1.

Why repeatedly make a copy of an entire array inside a loop over atoms, rather than copying the array before entering the loop? 2.

Why make a temporary local copy of the entire appropriate beadTypeIds_ array at all? All you need to do is assign appropriate values to atom type Ids. Why not just:

int typeId for (int i = 0; i < nAtom; ++i) { if (stateId == 0) { typeId = beadTypeIds0[i]; } else { typeId = beadTypeIds1[i]; } molecule.atom(i).setTypeId(beadTypeId); }

If you agree, please change and send another tiny pull request.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dmorse/simpatico/pull/48#issuecomment-370644731, or mute the thread https://github.com/notifications/unsubscribe-auth/AIEb6pHBHo00CuLPWIwAjHyFG_hXsnRoks5tbfqIgaJpZM4SRnp5 .

[dmorse]

Josh, That looks fine, and its also the most efficient option. -Dave

On Tue, Mar 6, 2018 at 10:38 PM, jmysona notifications@github.com wrote:

That's a very good point. The repeated copying of the array is excessive. I think the original intention was to avoid having to evaluate an if statement multiple times.

Currently I've gone with a third option which avoids the repeated evaluation of the if statement; the code checks to see which subtype it should set the molecule to, then loops over the atoms assigning them their new types. The code is now

int nAtom = molecule.nAtom(); if (stateId == 0) { for (int i = 0; i < nAtom; ++i) { molecule.atom(i).setTypeId(beadTypeIds0[i]); } } else { for (int i = 0; i < nAtom; ++i) { molecule.atom(i).setTypeId(beadTypeIds1[i]); } } setMoleculeStateId(molecule, stateId);

Let me know if this is acceptable

On Mon, Mar 5, 2018 at 8:55 PM, David Morse notifications@github.com wrote:

Josh, I merged the pull request, because nothing appeared wrong, but didn't understand why you chose to implement this function this way:

void LinearSG::setMoleculeState(Molecule& molecule, int stateId) { int nAtom = molecule.nAtom(); DArray beadIdentities; for (int i = 0; i < nAtom; ++i) { if (stateId == 0) { beadIdentities = beadTypeIds0; } else { beadIdentities = beadTypeIds1; } molecule.atom(i).setTypeId(beadIdentities[i]); } setMoleculeStateId(molecule, stateId); }

Questions:

1.

Why repeatedly make a copy of an entire array inside a loop over atoms, rather than copying the array before entering the loop? 2.

Why make a temporary local copy of the entire appropriate beadTypeIds_ array at all? All you need to do is assign appropriate values to atom type Ids. Why not just:

int typeId for (int i = 0; i < nAtom; ++i) { if (stateId == 0) { typeId = beadTypeIds0[i]; } else { typeId = beadTypeIds1[i]; } molecule.atom(i).setTypeId(beadTypeId); }

If you agree, please change and send another tiny pull request.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dmorse/simpatico/pull/48#issuecomment-370644731, or mute the thread https://github.com/notifications/unsubscribe-auth/ AIEb6pHBHo00CuLPWIwAjHyFG_hXsnRoks5tbfqIgaJpZM4SRnp5 .

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/dmorse/simpatico/pull/48#issuecomment-371020343, or mute the thread https://github.com/notifications/unsubscribe-auth/ABweIDjUNef8hKuhbyPjivJ_X5jQR1-oks5tb2QngaJpZM4SRnp5 .