Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent Reporting of Dihedral Energies #391

Closed
fjclark opened this issue Oct 19, 2022 · 5 comments
Closed

Inconsistent Reporting of Dihedral Energies #391

fjclark opened this issue Oct 19, 2022 · 5 comments
Labels

Comments

@fjclark
Copy link
Contributor

fjclark commented Oct 19, 2022

Hello,

The reporting of dihedral energies between AMBER and GROMACS seems to be inconsistent. For AMBER, getDihedralEnergy() reports the total dihedral energy, whereas in GROMACS, getDihedralEnergy() only reports the proper dihedral energy. This means that the dihedral energy for AMBER is the sum of the dihedral and improper energies for GROMACS:

I've attempted to show this using a BioSimSpace script, but getImproperEnergy() returns None for GROMACS, I'm guessing because IMPRPROPERDIH" should be "IMPROPERDIH" here:

return self.getRecord("IMPRPROPERDIH", time_series, _Units.Energy.kj_per_mol, block)

However, I can confirm this by checking the outputs.

The script and input files to reproduce are here: dihedral_issue.tar.gz.

It might be clearer if getDihedralEnergy in GROMACS and NAMD returned the total dihedral energy, and separate functions were created for querying the proper and improper components.

Thanks.

@fjclark fjclark added the bug label Oct 19, 2022
@lohedges
Copy link
Member

Hi there,

This is a known behaviour due to the way our parsers work. Apologies that this wasn't clearer. It's tricky because of the way that dihedrals are represented in different engines and how they are treated when interconverting between one format and another. In practice, both are dihedrals, and for AMBER, the current parser collects both propers and impropers together (as you see).

I'll have a think if there's an easy way to make things consistent. However, I think it would require quite a bit of reworking to several parsers.

Thanks for catching the typo in the GROMACS log file extractor. I'll update that when I get a chance.

@lohedges
Copy link
Member

Going forward I need to think of a way of flagging which get... methods are interoperable. Some are always present regardless of the engine, e.g. getSystem, but others are engine/protocol dependent, and might give different results depending on how the engine behaves.

@fjclark
Copy link
Contributor Author

fjclark commented Oct 19, 2022

OK, thank you for the clarification.

No problem.

@fjclark fjclark closed this as completed Oct 19, 2022
@fjclark
Copy link
Contributor Author

fjclark commented Oct 19, 2022

This is a known behaviour due to the way our parsers work. Apologies that this wasn't clearer. It's tricky because of the way that dihedrals are represented in different engines and how they are treated when interconverting between one format and another. In practice, both are dihedrals, and for AMBER, the current parser collects both propers and impropers together (as you see).

Apologies if I'm missing something, but it's still not clear to me why getDihedralEnergy() can't return the total dihedral energy, given that AMBER, GROMACS, and NAMD all support either finding the total dihedral energy, or both its proper and improper components. As in, why can't this line:

return self.getRecord("PROPERDIH", time_series, _Units.Energy.kj_per_mol, block)

Be changed to

return self.getRecord("PROPERDIH", time_series, _Units.Energy.kj_per_mol, block) + self.getRecord("IMPROPERDIH", time_series, _Units.Energy.kj_per_mol, block) 

and similarly for NAMD? This would prevent confusing behavior where changing the engine results in different output from getDihedralEnergy() (e.g. I'm assuming the discrepancy between dihedral angle energies you show here is because the improper energy isn't included for GROMACS #289 (comment)). Why couldn't the function getProperEnergy could then be implemented only for GROMACS and NAMD, as is getImproperEnergy?

Thanks very much!

@lohedges
Copy link
Member

No problem, that makes sense. I guess I would need to remove the getImproperEnergy methods, since they wouldn't always return the same result following interconversion, e.g. if you round-tripped. The total should be consistent, though.

return self.getRecord("PROPERDIH", time_series, _Units.Energy.kj_per_mol, block) + self.getRecord("IMPROPERDIH", time_series, _Units.Energy.kj_per_mol, block) 

Something like this should work, but the logic would need to be changed since records return None if not present, not 0, which could be a valid energy (in appropriate units).

I'll add a note to implement this when I get a moment.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants