Skip to content

Add configurable trajectory persistence and decouple thermo logging in melt-quench pipeline#395

Merged
Gitdowski merged 19 commits into
mainfrom
fix-avoid-storing-100-frames
Jul 14, 2026
Merged

Add configurable trajectory persistence and decouple thermo logging in melt-quench pipeline#395
Gitdowski merged 19 commits into
mainfrom
fix-avoid-storing-100-frames

Conversation

@Gitdowski

@Gitdowski Gitdowski commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

This PR (and its major reviews) improves melt-quench persistence and observability. It also seperates the NVT simulation required for structure analysis averaging if n_averaging_frames > 1 into the structure_analysis workflow.

  • Fixed n_averaging_frames plumbing so user-provided values are respected end-to-end.
  • Split n_print into n_dump and n_print_thermo to decouple thermo log verbosity from sparse dump cadence.
  • Updated also in docs and notebooks
  • Kept final_structure behavior unchanged for downstream analyses.
  • Improved database docstrings to clearly describe the trajectory dataflow and persistence policy.
  • Adding a trajectory_storing_mode independently for for melt-quench and structure analysis workflow to control what part of the trajectory/dump data enters the database:
    • all_frames_all_data keeps all data as recorded
    • all_frames_drop_velocities_and_forces keeps all dumped frames but removes potentially data-heavy forces and velocities
    • last_frame_all_data reduces dump-related data (key-names in parsed_output: natoms, cells, indices, forces, velocities, unwrapped_positions, positions) to only the last recorded frame
    • last_frame_drop_velocities_and_forces
  • Note that the trajectory_storing_mode is currently applied individually on all stages of the melt-quenching procedure. Therefore, if the last_frame option is selected, the simulation history will contain the last structure of every stage.
  • pixi: set lammpsparser version to >=0.0.7 to have access to new dump_final_structure parameter
  • unification of _run_lammps_md function to avoid requiring multiple definitions of it
  • Moving the additional NVT run of the melt quenching simulation (required for structure analysis) to the actual structure analysis workflow

@Gitdowski
Gitdowski requested a review from ltalirz June 17, 2026 08:29
@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

@Gitdowski Gitdowski added the integration Tag a PR with this to run integration tests label Jun 17, 2026
Comment thread amorphouspy/src/amorphouspy/fabrication/meltquench.py Outdated
Comment thread amorphouspy/src/amorphouspy/fabrication/meltquench.py Outdated
@ltalirz

ltalirz commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR! Happy to merge after the minor changes discussed

@Gitdowski

Copy link
Copy Markdown
Contributor Author

Working on a more unified version to avoid seperate definitions of the _run_lammps_md. Expect a major update of this PR today, also taking into account new option in lammpsparser that makes sure the last frame is dumped.

@Gitdowski

Gitdowski commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Hey @Atilaac and @ltalirz,
unifying the _run_lammps_md function (now with individual n_dump and n_print_thermo, also now accepting pressure, and making sure the last run of a simulation is always dumped via dump_final_structure = True) had quite some effect on various parts of the package. I tried to consistently use the new parameters everywhere, also on the api side, to avoid confusion.

I also updated the notebooks and copied the changed notebooks to docs/notebooks (@Atilaac: I guess this now also requires the builddocs label ? also any other?)

Please have again a look at the changes.

PS: I am out of office tomorrow. We can discuss today or next week.

Comment thread notebooks/MeltquenchTutorial.ipynb
Comment thread notebooks/MDTutorial.ipynb
trajectories.
2. On persistence, ``result_data['melt_quench']['simulation_history']`` is
extracted into the dedicated ``Job.simulation_history`` column.
3. Storage policy is controlled by ``persist_structures``:

@ltalirz ltalirz Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this more explicit

persist_trajectory:

  • "all":
  • "last_frame"

and be explicit that we just store positions and cells (note: the prepare_for_history function needs to be adapted to also drop forces/velocities in case all)

@ltalirz

ltalirz commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@Gitdowski You may also be able to close #340 once this is merged

- fix: storing only last frame for structure averaging
- fix: add independent n_dump & n_print_thermo intervalls
- fix: unify _run_lammps_md
- fix: add option to allow saving all dumps / keeping only last dump
- fix: move n_averaging_frames and NVT simulation to structure analysis
- fix: typing issue
- fix: avoid unit conversion warnings for lammps runs
- fix: tests & rerun notebooks
- fix: remove structure from deprecated data
- fix: improve docstrings
- chore: update pixi.lock (#396) & pixi.toml for lammpsparser 0.0.7
- perf: faster job:search
- perf: automatic number of cores
- perf: faster glasses:search exact matches
- perf: index created_at for fast date-range job search (#398)
@Gitdowski
Gitdowski force-pushed the fix-avoid-storing-100-frames branch from cc847ce to 386486c Compare July 7, 2026 11:51
Comment thread amorphouspy/src/amorphouspy/lammps/runner.py
Comment thread amorphouspy_api/src/amorphouspy_api/database.py
Comment thread amorphouspy/src/amorphouspy/properties/structural/all.py
@Gitdowski
Gitdowski merged commit 8d1c857 into main Jul 14, 2026
9 of 10 checks passed
@Gitdowski
Gitdowski deleted the fix-avoid-storing-100-frames branch July 14, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration Tag a PR with this to run integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

60k atom job failed with "string longer than INT_MAX bytes"

3 participants