Create observables based on Observable_stat and remove state tracking #3723
Conversation
Codecov Report
@@ Coverage Diff @@
## python #3723 +/- ##
======================================
- Coverage 88% 88% -1%
======================================
Files 549 551 +2
Lines 24684 24669 -15
======================================
- Hits 21754 21734 -20
- Misses 2930 2935 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM except for minor changes. Maybe @RudolfWeeber can have a look on the interfaces changes, also somebody else needs to sign off on the stress tensor name change ;-)
|
@RudolfWeeber would you like to have a look? The API change is limited to renaming the |
|
Yes, I’ll review the PR.
|
|
If we use "pressure tensor"now, that name should be used also in the espressomd.analysis module. Otherwise, it'd be a bit confusing. Where does the energy of the Homogeneous magnetic field go? If I understand the Pr correctly, this is no longer separable from the dipole-dipole energy? That's probably not ideal. |
Indeed, this PR merges all dipolar contributions into a single field. Electrostatic and magnetostatic methods were storing their various contributions into unlabeled slots, which only works if contributors know exactly what they are doing when they write Looking again at the diff, it seems the dipolar energies are written to the fields following a pattern, e.g. magnetic field-dipole and dipole-dipole interactions in
I would prefer not to modify this part of the At the moment, the PressureTensor/Pressure/Energy observable framework is still work in progress. What about the following: once we're done with the observables infrastructure, we'll let ICP members know they should consider updating their scripts to use observables while the analysis module remains for a grace period, as we did for steepest descent (that grace period would end before the release deadline). For as long as the observables framework remains a WIP, I'd be fine with naming |
|
Indeed, this PR merges all dipolar contributions into a single field. Electrostatic and magnetostatic methods were storing their various contributions into >unlabeled slots, which only works if contributors know exactly what they are doing when they write obs_energy.local.dipolar[0] += energy;. The >python interface used to give the different contributions with the integer index of the field, which can only be interpreted by those familiar with the C++ code.
Looking again at the diff, it seems the dipolar energies are written to the fields following a pattern, e.g. magnetic field-dipole and dipole-dipole interactions in >[0], dipolar solver in [1], corrections in [2]. If we revert back to the old behavior, we should at least document this somewhere, as it wasn't obvious to me.
I’d keep the individual ones, but as you suggest, change the key names in the return dict of analsys.energy()
If we use "pressure tensor"now, that name should be used also in the espressomd.analysis module. Otherwise, it'd be a bit confusing.
I would >prefer not to modify this part of the espressomd.analysis module as it would break everyone's scripts. The long-term plan is to replace this module >with observables anyway, so there's no need for developers and users to spend effort in adapting code and scripts to the new convention in analysis, only to have >to spend time again adapting scripts to observables once we remove the feature from the analysis module.
At the moment, the PressureTensor/Pressure/Energy observable framework is still work in progress. What about the following: once we're done with the >observables infrastructure, we'll let ICP members know they should consider updating their scripts to use observables while the analysis module remains for a >grace period, as we did for steepest descent (that grace period would end before the release deadline). For as long as the observables framework remains a WIP, >I'd be fine with naming PressureTensor back to StressTensor.
I think using the same name for the same thing is preferable, even if users have to change it in their script. We had a few non-silent api changes in previous minor versions as well, e.g., with random seeds and with LB setup.
There is that issue that people seem to use the opposite sign convention for stress, i.e., positive number means that the system wants to contract.
So, I vote for renaming to pressure tensor everywhere.
Than it should also be clear that pressure and pressure tensor use the same sign convention.
|
Recalculate energy, scalar pressure and pressure tensor whenever the corresponding python analysis module functions are called.
Return only the total pressure tensor, scalar pressure, energy. Rename StressTensor to PressureTensor since it has the sign of a pressure.
Fixes -Wnonportable-include-path on AppleClang 9 (non-portable path to file '"Energy.hpp"'; specified path differs in case from file name on disk: #include "energy.hpp").
Avoids a side effect when the first test class fails, leaving the system in an unclean state for the next test class.
jngrad
force-pushed
the
obsstat-observable
branch
from
7379d83 to
072180f
Compare
Done everywhere in c5a19f4. I've also made the change in exported functions of the LB code. There were a few comments in the LB code and LB tutorials to remind developers that LB stress was in fact LB pressure.
I've removed the commits that merged dipole slots into a single field. Same for electrostatics. These slots are now documented in 072180f. The rest of the commits have been slightly touched to resolve conflicts from the removal. |
|
@RudolfWeeber can you please check if your points have been addressed? |
|
For what its worth, I think it is a mistake to keep the separate long-rang and short-range contributions from the dipolar and electrstatics solvers exposed. Using them separately is almost always a mistake, |
Fixes #2200, partial fix for #3718
Description of changes:
Observable_statObservablesclassesPressureTensor,Pressure,Energy