Also the fix is in(tm), but I need to address gpu_update_box and I need to
know if ucell matrices are still upper diagonal because if not that will
require some further work as I had to update the cell offsets to be
precalculated to make fixed point consistent and properly bounded so there
can't ever be an overflow.
On Fri, Mar 18, 2022 at 12:17 PM Scott Le Grand <varelse2005.gmail.com>
wrote:
> 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:04 PDT
