[QC-885] Modify reference check to also accept a TH1 … #2358
Conversation
… of a Canvas - also pass the database object from CheckRunner to CheckInterface and use it to implement a new method `retrieveReference`.
Barthelemy
changed the title
|
@Barthelemy shall I test it locally and report back? |
|
@aferrero2707 good idea ! thanks |
|
@Barthelemy I have loaclly compared the direct check you implemented with the check result from the ReferenceComparator output, and they perfectly match, so as far as I can tell the code is working as expected. Here is a screenshot showing the result of the two check methods: |
|
@aferrero2707 Thanks a lot for checking. I have resolved the conflict as well. @knopers8 I think that you can review it if you want |
|
ok, i will have a look now |
| protected: | ||
| /// \brief Called each time mCustomParameters is updated. | ||
| virtual void configure() override; | ||
|
|
||
| /// \brief Retrieve a reference plot at the provided path, matching the give activity and for the provided run. | ||
| /// the activity is the current one, while the run number is the reference run. | ||
| std::shared_ptr<MonitorObject> retrieveReference(std::string path, int referenceRun, Activity activity); |
There was a problem hiding this comment.
If there is only one correct/expected Activity that the user shall provide and the framework has the knowledge of it, why even ask the user to do it? Shouldn't the framework handle it?
Also, since the run numbers are unique, why even constrain the selection with the Activity? I don't understand what's the benefit, except that someone would want to fail if the reference run is from an earlier period than the current one. However, for such cases we should anyway produce more specific errors than could not find object, so we should handle it explicitly.
There was a problem hiding this comment.
@aferrero2707 I picked these parameters based on your code. Do you have an opinion?
I would tend to agree with Piotr.
There was a problem hiding this comment.
@Barthelemy @knopers8 indeed, I also agree. I would say that we need two parameters from the activity:
- the provenance, as we do not want to mix online and async plots
- the pass name, but that's only relevant for async, where the mechanism of reference runs might be a bit more difficult to put in place
There was a problem hiding this comment.
After discussing with Andrea as well, I came to the conclusion that we should keep the parameter activity, at least for the time being. Replacing it with provenance and pass opens the door to having more and more parameters that will match what the Activity contains in the end. Moreover, we cannot let the framework feed the activity because the CheckInterface does not store the activity. We could modify all the *Interface to start storing the activity but I believe that it would go beyond the scope of this PR.
What I did was to document what the activity parameter is.
Co-authored-by: Piotr Konopka <piotr.jan.konopka@cern.ch>
Co-authored-by: Piotr Konopka <piotr.jan.konopka@cern.ch>
Co-authored-by: Piotr Konopka <piotr.jan.konopka@cern.ch>
Co-authored-by: Piotr Konopka <piotr.jan.konopka@cern.ch>
- update the namespaces
|
@Barthelemy @knopers8 I basically agree on all suggestion from Piotr. I have also added a comment d=regarding the way the reference run number is retrieved in the checker, suggesting to make use of the extended check parameters for that. For the rest, looks good to me. |
| protected: | ||
| /// \brief Called each time mCustomParameters is updated. | ||
| virtual void configure() override; | ||
|
|
||
| /// \brief Retrieve a reference plot at the provided path, matching the give activity and for the provided run. | ||
| /// the activity is the current one, while the run number is the reference run. | ||
| std::shared_ptr<MonitorObject> retrieveReference(std::string path, int referenceRun, Activity activity); |
There was a problem hiding this comment.
@Barthelemy @knopers8 indeed, I also agree. I would say that we need two parameters from the activity:
- the provenance, as we do not want to mix online and async plots
- the pass name, but that's only relevant for async, where the mechanism of reference runs might be a bit more difficult to put in place
- use size_t for the run number - use Timestamp::Latest
|
@knopers8 as discussed on Friday, I have replaced the |
… instead of a Canvas
retrieveReference.