Observable objects #738
Observable objects #738
Conversation
lfarv
force-pushed
the
observables
branch
2 times, most recently
from
59d4a1b
to
d3584bc
Compare
There was a problem hiding this comment.
@lfarv, I started looking at this proposal, and stopped because I think the overall design needs to be improved starting from the decision made for the Variables class.
I would implement a true base class, that does not need any ring and implements an abstract method for the evaluation function such that a signature is not imposed at this stage.
A few details in the code review.
Then I have a question, why defining "Private classes"? What is the motivation for that?
For the rest it seems ok but we can come back to it once the base design is fixed
pyat/at/latticetools/observables.py
Outdated
| """Setup function called when the observable is added to a list""" | ||
| pass | ||
|
|
||
| def evaluate(self, ring: Lattice, *data, initial: bool = False): |
There was a problem hiding this comment.
The base class is general, is ring really needed to evaluate an observable? I would make it optional
There was a problem hiding this comment.
Similarly why do we request data here?
There was a problem hiding this comment.
"ring" does not appear anymore the definition of the Observable class.
But data is still there: it's necessary for most evaluation functions, and depends on the needs argument. It's empty if no need is specified.
I have no more example of use of this basic observable. Do you have an idea of such an observable which does not depend on ring? Now for observables depending an a ring, it's easier to use more specific classes like RingObservable of ElementObservable
| GEOMETRY = 10 | ||
|
|
||
|
|
||
| class Observable(object): |
There was a problem hiding this comment.
Could be done… But since this can be used directly by users, I find the simple Observable name clearer. I can change if you find ObservableBase better…
| super().__init__(fun, refpts, needs=needs, name=name, **kwargs) | ||
|
|
||
|
|
||
| class _GlobalOpticsObservable(Observable): |
There was a problem hiding this comment.
Again why this notation?
There was a problem hiding this comment.
Because it's not supposed to used directly. If we want it public, on has to find new names for both _GlobalOpticsObservable and GlobalOpticsObservable.
|
@swhite2401: I think all your comments have been addressed. Now do you have ideas for user-defined observables for
I would be nice to put these in the documentation! |
This PR introduces
Observableobjects. Together withVariableobjects, they provide a unified and expandable set objects which can be used in matching, response matrices, plots, parameter scans…ObservableListsoptimise the performance in evaluating the observables by grouping the requirements of observables and limiting the computations to the strict minimum.The initial set of
Observablesprovided here gives access to most of lattice properties. It can be easy extended by providing custom evaluation functions or inheriting from the basic set.A documentation preview is here, and a demo notebook is provided.