Reshape output of Observables #3560
Conversation
Codecov Report
@@ Coverage Diff @@
## python #3560 +/- ##
=======================================
+ Coverage 87% 87% +<1%
=======================================
Files 525 524 -1
Lines 23732 23603 -129
=======================================
- Hits 20793 20769 -24
+ Misses 2939 2834 -105
Continue to review full report at Codecov.
|
|
On Tue, Mar 03, 2020 at 05:45:41AM -0800, Jean-Noël Grad wrote:
jngrad commented on this pull request.
> from .script_interface import ScriptInterfaceHelper, script_interface_register
@script_interface_register
class Observable(ScriptInterfaceHelper):
_so_name = "Observables::Observable"
- _so_bind_methods = ("calculate", "n_values")
+ _so_bind_methods = ("_calculate", "shape")
just tried it, but
```python
def calculate(self):
return np.array(self.call_method("calculate")).reshape(self.shape())
```
is overshadowed by the `self.calculate()` method created by the `ScriptInterfaceHelper`.
Just to be sure:
You can ommit the calculate in bind_methods. This is just a short way of manually implementing
```
def calculate(...):
self.call_method("calculate", {})
```
|
|
@RudolfWeeber This works, thanks! We discussed the naming convention internally and chose to use the underscore prefix because it allows users to add the underscore prefix in their old 4.1 scripts to get the old behavior back, without having to rework the numpy reshape logic (for example the LB stress tensor requires more than just a reshape operation). This info would only be mentioned in the release notes, and the |
|
On Tue, Mar 03, 2020 at 06:32:46AM -0800, Jean-Noël Grad wrote:
@RudolfWeeber This works, thanks! We discussed the naming convention internally and chose to use the underscore prefix because it allows users to add the underscore prefix in their old 4.1 scripts to get the old behavior back, without having to rework the numpy reshape logic (for example the LB stress tensor requires more than just a reshape operation). This info would only be mentioned in the release notes, and the `_calculate` method would then be removed in espresso 4.3 or 5.0 using some strategy (for example yours). Do you think we should use your solution right now for 4.2?
Not sure, I understand.
In my opiniion, in 4.2., observables should return the correct shape when using .calculate().
That probably includes the stress tensor. For total stress, it is anyway not guaranteed that it is symmetric, and for lb stress, the format is confusing and error-prone. I'd change that toa a 3x3 matrix as well.
I don't care, whether or not the old behavior stays available as _calculate(), although I'd personally not do it, because people have to adapt their scripts anyway. Then, adapting to the final (physically reasonable) return shape would be best, imo.
|
The LB stress tensor is now returned as a symmetric matrix instead of a flattened lower triangle.
I'll remove the binding to the old method. |
Fixes #2233
Description of changes: