First up, thanks for replying! Appreciated!
But... For triclinic cells, I never saw a ucell matrix full occupied.
u(2,1), u(3,1), and u(3,2) are always zero, yes?
If so, then the column space defines a, b, and c by the respective column
lengths that *are* the distances between those separating planes. Check it
out for yourself. Can you point me to a case that violates this? If so, we
have to fix the whole shebang, not just this routine. Because I agree with
you if we have fully occupied ucell matrices, otherwise no.
On Fri, Mar 18, 2022 at 12:10 PM David Cerutti <dscerutti.gmail.com> wrote:
> What's going on here is that not all unit cells are orthorhombic--such
> would imply that the box angles alpha, beta, and gamma are always 90
> degrees. The general unit cell is triclinic, where alpha, beta, and gamma
> can take on any values (with some degree of excluded territory, as certain
> combinations would fail to make a shape that can exist). The code you are
> seeing are seeing was created in response to another aspect of the original
> pmemd.cuda pair list that, if I recall correctly, was missing interactions
> from time to time due to a miscommunication between the way the pair list
> was generated versus the way the box had changed. I don't recall if it was
> creating energy drift or causing an outright crash. It was also around the
> time I added the -AllowSmallBox flag: note that the code will not allow
> users to run simulations that are less than three pair list subdivisions
> along any dimension without an explicit instruction to do so, as I wasn't
> sure that the way the particles get re-imaged at the beginning of each time
> was safe if there are only two subdivisions.
>
> But the HessianNorms function is calculating the actual thickness of each
> subdomain cell, as defined by the distance separating the planes of
> opposing faces. This is integral to the reason that, unless you do some
> sophisticated stuff like the honeycomb idea I had, a truncated octahedron
> is not as efficient as the 22% reduction in overall particle count would
> imply: the total volume of each cell needs to expand as it tilts, and this
> implies more tiles to cover the same volume. It's still advantageous to
> use a truncated octahedron, as the Hilbert Space curve does pretty well to
> localize what is needed and of course the cutoff shell of any given
> particle remains spherical.
>
> What's the ETA on the energy drift fix? Is this an atomic change, or does
> it also come with other refactoring / rollback of various precision trims?
>
> Dave
>
>
> On Fri, Mar 18, 2022 at 2:14 PM Scott Le Grand <varelse2005.gmail.com>
> wrote:
>
> > Let's look at this, here's the new code:
> > HessianNorms(invU, cdepth);
> > double skin = cdepth[0]/gpu->sim.xcells - gpu->sim.cut;
> > double yskin = cdepth[1]/gpu->sim.ycells - gpu->sim.cut;
> > double zskin = cdepth[2]/gpu->sim.zcells - gpu->sim.cut;
> >
> > The array cdepth is filed with the new a, b, and c by means of an
> external
> > call. But as far as I know, all unit cells are orthorhombic, so we've
> > always been able to use:
> > // Recalculate a, b, c
> > double a =
> > pNTPData->ucell[0];
> > double b =
> > sqrt(pNTPData->ucell[1] * pNTPData->ucell[1] +
> >
> > pNTPData->ucell[4] * pNTPData->ucell[4]);
> > double c =
> > sqrt(pNTPData->ucell[2] * pNTPData->ucell[2] +
> >
> > pNTPData->ucell[5] * pNTPData->ucell[5] +
> >
> > pNTPData->ucell[8] * pNTPData->ucell[8]);
> >
> > because of the constraints that imposes on the column space of the unit
> > cell matrix.
> >
> > So either the first code clause is overkill and repeated work or we have
> to
> > revisit the previous orthorhombic assumption. Which is it?
> >
> > But even then, why would one change the unit cell matrix without updating
> > the a, b, and c derived from it? And yes, I'm impatient w/r to fixing
> bugs.
> > Guilty.
> >
> >
> > On Fri, Mar 18, 2022 at 10:42 AM Scott Le Grand <varelse2005.gmail.com>
> > wrote:
> >
> > > If the unit cell changes, wouldn't a, b, and c change as well since
> they
> > > are derived from said unit cell?
> > >
> > > As for the speed of response. If it's important to anyone, they'll
> > > respond. If not, well they can make their principled stand against me
> and
> > > my impatience later. Otherwise, I acknowledge your point but also my
> > > okeydokey here, nobody is debugging this thing. It's an easy fix. And I
> > > need to do that fix to make it exploit the fixed point local coordinate
> > > system, but I could leave the seemingly sloppy incorrect behavior here.
> > > Driveby contributions to PMEMD like this however are just not good IMO
> > and
> > > why I threw my hands up in frustration after trying to refactor for
> > > performance in 2020. It's a Sisyphusian effort and the tsunami of
> > > unattended warning messages and dead code in the thing alone is enough
> to
> > > make one run screaming into the night IMO. It was a single unitialized
> > > variable that broke PMEMD for Silicon Therapeutics in 2020. But also,
> why
> > > is there is so much dead code?
> > >
> > > The author of this code added a new and completely superfluous method
> of
> > > recalculating a, b,and c without exploiting the properties of the
> > > orthorhombic coordinate system to do it more simply as it is done in
> > > gpu_pressure_scale, which itself does the same thing internally as
> > > gpu_update_box if there's an MC barostat. And I hope you can forgive me
> > for
> > > being annoyed at the needless repetition of functionality here w/o any
> > > notes as to why this was necessary.
> > >
> > > On Fri, Mar 18, 2022 at 10:22 AM David A Case <david.case.rutgers.edu>
> > > wrote:
> > >
> > >> On Fri, Mar 18, 2022, Scott Le Grand wrote:
> > >>
> > >> >Okeydokey, nobody cares. Therefore it's a bug and I'm going to fix
> it.
> > >> Test
> > >> >will be updated to reflect this. What SM revision should I use for
> the
> > >> test
> > >> >reference files?
> > >>
> > >> ??!? How fast do you expect people to respond? I looked at this and
> > >> didn't
> > >> see any obvious error -- the two routines are called at different
> > places:
> > >> gpu_pressure_scale only when ntp>0 and barostat=1, whereas
> > gpu_update_box
> > >> is only called when re-weighting trajectories.
> > >>
> > >> But I don't know the code here. Clearly, you must be convinced that
> > a,b,c
> > >> need to be updated when reweight is > 0. I'll try to look at what
> > >> behavior
> > >> changes when you make the change. But I'm travelling to the ACS
> meeting
> > >> tomorrow, so you may disappointed if you require immediate feedback.
> > >>
> > >> ....dac
> > >>
> > >>
> > >> _______________________________________________
> > >> AMBER-Developers mailing list
> > >> AMBER-Developers.ambermd.org
> > >> http://lists.ambermd.org/mailman/listinfo/amber-developers
> > >>
> > >
> > _______________________________________________
> > AMBER-Developers mailing list
> > AMBER-Developers.ambermd.org
> > http://lists.ambermd.org/mailman/listinfo/amber-developers
> >
> _______________________________________________
> AMBER-Developers mailing list
> AMBER-Developers.ambermd.org
> http://lists.ambermd.org/mailman/listinfo/amber-developers
>
_______________________________________________
AMBER-Developers mailing list
AMBER-Developers.ambermd.org
http://lists.ambermd.org/mailman/listinfo/amber-developers
Received on Fri Mar 18 2022 - 12:30:03 PDT
