Fix melt detection and pressure convergence - #275
Open
srmnitc wants to merge 6 commits into
Open
Conversation
…mode tolerance.solid_fraction defaulted to 0.0 and liquid_fraction to 1.0 in 1.8.3 (1e7b076). The measured solid fraction is bounded to [0, 1], so those values make check_if_melted (frac < 0.0) and check_if_solidfied (frac > 1.0) unreachable: MeltedError and SolidifiedError can never be raised. That is fine for fe/ts runs, which only lose a safety net, but it breaks mode melting_temperature outright. MeltingTemp.run_jobs signals "solid melted" / "liquid froze" purely through those two exceptions, and start_calculation walks the temperature bracket on the resulting return codes 2 and 3. With both exceptions dead the bracket is never adjusted, so the first guess is accepted unconditionally and Tm is reported without ever verifying that the solid stayed solid and the liquid stayed liquid. Keep the defaults off, and instead have MeltingTemp turn the checks on for the two sub-calculations it generates, logging a warning when it does. An explicit user setting is still respected. Every other mode now logs a warning when the check relevant to its reference phase is unreachable, so the condition is visible in calphy.log after the fact.
… the stored box run_pressure_convergence accumulates avg.dat across cycles and, once the linear P-V fit fires, issues change_box to resize the cell. avg.dat was never rebased on that rescale, so from then on the mean pressure, the stored lx/ly/lz and the pv_history points all averaged over samples taken at two or more different volumes. Two consequences. The convergence test could be satisfied by stale high-pressure samples cancelling fresh low-pressure ones -- the same class of error the transient skip was introduced to remove, re-entering through the rescale path. And self.lx/ly/lz, the box handed to every downstream free-energy stage, became a historical mean over the search trajectory rather than the box at the target pressure. Track the index at which the current box's samples begin and slice from there, rather than truncating: avg.dat may be spread over per-segment parts that the runner concatenates on read, and driver code does not own those paths. This keeps the executable and library backends identical and emits no new LAMMPS commands, so the golden streams are unchanged. Also refer the scale factor to the volume the box actually has. change_box scales the current cell, but the factor was computed against pv_history's last entry -- a windowed mean that lags the box whenever it is still moving, which is exactly when the fit runs. The cycle straight after a rescale has had no equilibration at the new box, so it is discarded rather than allowed to decide convergence or feed the fit. Cycle 1 of the loop is unaffected: it follows a full equilibration run.
The Tm error bar needs the rate at which the solid and liquid free-energy curves separate at their crossing, taken as a symmetric finite difference at arg +/- 50. find_tm only rejects arg == 0 and arg == len - 1, so nothing kept that stencil inside the array. Above the top edge arg + 50 runs past the end and raises IndexError -- in postprocessing, after every MD step has already been paid for, so a long melting-point run dies with no result. Below the bottom edge arg - 50 goes negative and numpy wraps it to the far end of the sweep, silently replacing the local slope with a chord across the whole temperature range; the reported uncertainty then comes out ~1.4% wrong on a realistically curved F(T) with no indication anything went wrong. The stencil was also a fixed width against an array whose length is _n_sweep_steps, so a short sweep made this the common case rather than an edge case: for a 101-sample sweep, 49 of the 99 interior crossings raise. Clamp the stencil to the array and scale its half-width with the sweep length, and return nan with a logged warning where a slope cannot be formed at all (too few samples, or curves parallel at the crossing) rather than dividing by zero. Tm itself is unchanged -- it is solres[0][arg] and never depended on the stencil.
ts.forward_<i>.dat / ts.backward_<i>.dat are written by fix print 1, one row per MD step, so a 50000-step sweep is ~3.7 MB per file. gather_results loaded all of them for every ts/tscale calculation and kept four full-length arrays per replica in object-dtype columns. Measured over 12 calculations at 50000 steps: 0.22 s / 9.7 MB peak before, 1.01 s / 62.5 MB peak after, with 19.2 MB retained in the frame (1.60 MB per calculation, scaling linearly with n_iterations). A phase-diagram harvest of a couple of hundred folders therefore carries several hundred MB it usually has no use for, and every copy of the frame multiplies it. Worse, the loads were unguarded, so one damaged file took down the whole gather -- a job killed mid-sweep leaves a ragged final row, and the resulting ValueError lost every other calculation in the folder too. That is a regression in robustness: before, these files were never opened. Put the block behind include_sweep_data (default off), add sweep_data_stride to thin the samples when it is on, and read each calculation inside a try/except that records the failure in error_code and warns instead of propagating. The columns stay in the schema either way so the frame shape does not depend on the flag. The columns are not in any release, so nothing downstream depends on the old default.
A ts sweep whose solid melts partway through still returns a free energy. The forward and backward integrals no longer describe the same system, so that number is wrong by an amount averaging cannot remove, and a melting point taken from it is wrong with it. This is what a Ag melting_temperature run hit: the solid sweep dissipated 2.3e-2 eV/atom against 1.7e-4 for the same sweep on an older version, and nothing said so. There was already a warning here, hardcoded at 1e-4 eV/atom -- below the 1.7e-4 of a perfectly healthy sweep, so it fired on essentially every run and had become noise. Replace it with tolerance.dissipation, defaulting to 1e-3 eV/atom: one to two orders of magnitude above a clean sweep, an order below a sweep that changed phase. 0 disables the check. The threshold has a physical reading rather than being tuned to the two measurements. Dissipation enters the free energy directly, so on a melting point it converts to roughly dissipation / dS_fus in temperature; at a typical dS_fus of ~1.2 kB the default is about 10 K, and the message quotes that figure for the observed value. For the run above it reports ~222 K, against an observed discrepancy of ~200 K. The verdict travels with the number: report.yaml gains results.ts_dissipation_high and gather_results exposes it as a column, so a harvest can be filtered without re-deriving the threshold. MeltingTemp repeats the warning in its own log, since the solid and liquid sweeps log into simfolders nobody reads when the point of the mode is one number.
.bumpversion.cfg still carried current_version = 1.8.5 while the tree was already at 2.0.0, so `bumpversion patch` would have produced 1.8.6. Corrected alongside the bump.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #274