Skip to content

Parse input files for DT when not in traj#71

Merged
ljwoods2 merged 3 commits intoimdreader-splitfrom
test_dt
Jun 25, 2025
Merged

Parse input files for DT when not in traj#71
ljwoods2 merged 3 commits intoimdreader-splitfrom
test_dt

Conversation

@ljwoods2
Copy link
Copy Markdown
Collaborator

@ljwoods2 ljwoods2 commented Jun 24, 2025

Fixes #70

Changes made in this Pull Request:

  • Parse MDP file from GMX using regex to get DT
  • Parse input file from LAMMPS using regex to get its DT
  • Remove some unused code (now that we always test for time, step, dt)

The GROMACS simulation engine test_compare_imd_to_true_traj is going to fail until we merge this with #53, which adds MinimalReader and allows keeping the IMD stream's DT value (instead of replacing it with time between transmitted frames as a result of the TRR reader)

Same goes for LAMMPS

(sorry for the black diff, apparently when I wrote everything previously I had configured a nonstandard, short max line length in my vscode)

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.46%. Comparing base (56430e9) to head (73556cc).

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ljwoods2 ljwoods2 requested review from Copilot and orbeckst June 24, 2025 05:26
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds functionality to parse the DT value from input files for GROMACS and LAMMPS using regular expressions, and removes obsolete code related to testing for time, step, and dt.

  • Updated test files for NAMD, LAMMPS, and GROMACS to use regex extraction for dt.
  • Removed unused fixtures and refactored test_compare_imd_to_true_traj to simplify dt handling.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
imdclient/tests/test_namd.py Reformatted logger formatter and test function definition style
imdclient/tests/test_lammps.py Added regex-based dt extraction and reformatted formatter setup
imdclient/tests/test_gromacs.py Added dt fixture using regex for mdp file and reformatted logger
imdclient/tests/base.py Refactored dt handling in tests by replacing old fixtures

Comment thread imdclient/tests/base.py
true_u.trajectory[i].time,
imd_u.trajectory[i - first_frame].time,
atol=1e-03,
)
Copy link

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

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

The call to assert_allclose includes three array arguments; the extra argument (imd_u.trajectory[i - first_frame].dt) should be removed so that the function compares only true_u.trajectory[i].dt and dt.

Copilot uses AI. Check for mistakes.
@amruthesht
Copy link
Copy Markdown
Contributor

@ljwoods2, two things -

  1. Maybe we can use the imdreader-split branch to introduce these changes?
  2. I think it would be a better idea to parse the *.log output file to get this information rather than the input file. Since that would confirm the actual value being used rather than the one that was set by the user.

@orbeckst orbeckst mentioned this pull request Jun 24, 2025
4 tasks
@ljwoods2
Copy link
Copy Markdown
Collaborator Author

ljwoods2 commented Jun 24, 2025

@amruthesht is there ever a case where dt would change from what the user set it to? Unless there's a clear case that we'd encounter I don't see value in rewriting it, if we ever want to drastically modify these tests in the future to include such a case we can do it then.

I'd be happy to swap this PR to make it into your imdreader-split branch, will do that late tonight.

@amruthesht
Copy link
Copy Markdown
Contributor

amruthesht commented Jun 24, 2025

@ljwoods2 - I can't think think of a specific case that is common so I think it should be fine for now to stick with parsing the input file.
The suggested change should be convenient though since the parsed .mdp file is just pasted out in the eventual *.log output for GROMACS

However, right now the gmx tests seem to be failing and still using the MDA "transmission dt" for some reason. I can take a stab at it if you want.

@ljwoods2
Copy link
Copy Markdown
Collaborator Author

@amruthesht that's expected, read the issue description. This doesn't use your minimalreader, so IMD universe is created by dumping stream into a TRR file and then rereading (which loses DT information)

@amruthesht
Copy link
Copy Markdown
Contributor

@amruthesht that's expected, read the issue description. This doesn't use your minimalreader, so IMD universe is created by dumping stream into a TRR file and then rereading (which loses DT information)

My bad - I thought you were doing comparisons without dumping it as a TRR.
This makes sense then to include in the imdreader-split branch

@ljwoods2 ljwoods2 changed the base branch from main to imdreader-split June 25, 2025 04:27
@ljwoods2 ljwoods2 merged commit b5bd892 into imdreader-split Jun 25, 2025
1 check passed
@ljwoods2 ljwoods2 deleted the test_dt branch June 25, 2025 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DT not tested in GROMACS, LAMMPS

4 participants