Skip to content

Conversation

@HanatoK
Copy link
Member

@HanatoK HanatoK commented Oct 17, 2025

This PR aims to complete #850 by adding the tests at first.

However, I was not quite sure how to add new tests with only enableFitGradients changed to yes, so I copied the files into input_files manually. Please let me know if you have a better idea to change the test files while keeping the old enableFitGradients no configuration for the time being.

Update:
For the time being, in commit 2691805, I have added a warning for enableFitGradients no.

Tasks:

  • Remove the tests with enableFitGradients no from the stub tests;
  • Update the NAMD tests;
  • Update the GROMACS tests;
  • Update the LAMMPS tests.

@giacomofiorin giacomofiorin added the testing Only affects CI; not listed in outside PRs label Oct 21, 2025
@jhenin
Copy link
Member

jhenin commented Oct 22, 2025

Looking at this, I wonder if we could just switch this flag on for all functional tests. That would be simpler.

@HanatoK
Copy link
Member Author

HanatoK commented Oct 22, 2025

Looking at this, I wonder if we could just switch this flag on for all functional tests. That would be simpler.

Honestly I don't know what to do with enableFitGradients. To phase out enableFitGradients no,
we need to update all reference data for all MD backends, and update the documentation to tell the users that enableFitGradients no is ignored, but perhaps some users would still argue that in their calculations the fit gradients could be canceled out and they want to skip them.

@jhenin
Copy link
Member

jhenin commented Oct 23, 2025

Yes, I'm in favor of removing it from the tests and regenerating all reference data that requires it.

We don't need to remove the keyword entirely. We can deprecate it by removing its entry from the documentation and adding a release note, e.g. on a dedicated Wiki page. So we won't break anyone's workflow, but we will simplify the work of new users who won't have to think about it.

@HanatoK HanatoK force-pushed the new_tests_enablefitgradients branch from 998958a to 2691805 Compare November 17, 2025 19:15
@jhenin
Copy link
Member

jhenin commented Nov 18, 2025

I'm still favorable to deprecating the keyword and testing only the fit gradient case.

@HanatoK
Copy link
Member Author

HanatoK commented Dec 2, 2025

Hi @jhenin and @giacomofiorin ! Should I regenerate the test data without enableFitGradients no for all backends in this PR, or in a separate one?

@jhenin
Copy link
Member

jhenin commented Dec 2, 2025

I would use this PR, for simplicity. You can change the title.

@HanatoK HanatoK changed the title test: add new tests with "enableFitGradients yes" Deprecate "enableFitGradients no" Dec 2, 2025
@HanatoK
Copy link
Member Author

HanatoK commented Dec 2, 2025

I would use this PR, for simplicity. You can change the title.

OK. I have changed the title and added a list of TODOs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Only affects CI; not listed in outside PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants