Pcvl 846 incompatible heralds and detectors should not be simulated #519
Conversation
| @@ -332,6 +334,9 @@ def samples(self, | |||
| * physical_perf is the performance computed from the detected photon filter | |||
| * logical_perf is the performance computed from the post-selection | |||
| """ | |||
| if not self.checker.check_heralds_detectors(self._heralds, self._detectors): | |||
| return {"results": BSSamples(), "physical_perf": 1, "logical_perf": 0} | |||
There was a problem hiding this comment.
"physical_perf": 1 c'est de la triche, non ?
There was a problem hiding this comment.
Oui, complètement, mais j'ai même pas envie de la calculer dans ce cas là
|
|
||
| @staticmethod | ||
| def check_heralds_detectors(heralds: dict[int, int] | None, detectors: list[IDetector | None] | None) -> bool: | ||
| if heralds is not None and detectors is not None: |
There was a problem hiding this comment.
if heralds and detectors:, non ?
(et plus bas aussi)
| for k, v in heralds.items(): | ||
| detector = detectors[k] | ||
| if detector is not None: | ||
| max_val = detectors[k].max_value |
There was a problem hiding this comment.
detectors[k] => detector ?
tests/test_processor.py
Outdated
| p.min_detected_photons_filter(0) | ||
| p.with_input(BasicState([1, 1])) | ||
|
|
||
| with LogChecker(mock_warn): |
There was a problem hiding this comment.
Je pense que ce test ne devrait vérifier que le comportement de check_heralds_detectors(...).
En l'état n'importe quel warning du simulateur va cacher une éventuelle erreur dans check_heralds_detectors.
| from perceval.utils.logging import channel, get_logger | ||
|
|
||
|
|
||
| class SimulatorChecker: |
There was a problem hiding this comment.
This class is not usefull, you should put check_heralds_detectors as a stand alone method (maybe in the detector file ?)
perceval/components/detector.py
Outdated
| @@ -55,6 +55,7 @@ class DetectionType(Enum): | |||
|
|
|||
|
|
|||
| class IDetector(AComponent, ABC): | |||
| max_value: int | |||
There was a problem hiding this comment.
max_value is a very generic name for this variable, consider max_detected_photons or max_detections
There was a problem hiding this comment.
/!\ max_value is here a python class attribute and not a property
(hopefully it has no consequences here but it could if we implement a new detector class and forgot to declare the max_value property)
you should do:
from abc import ABC, abstractmethod
...
class IDetector(AComponent, ABC):
@property
@abstractmethod
def max_value(self):
pass
tests/test_sim_checker.py
Outdated
| @patch.object(pcvl.utils.logging.ExqaliburLogger, "warn") | ||
| def test_incompatible_heralds_slos(mock_warn): | ||
| checker = SimulatorChecker() | ||
| detector_th = Detector.threshold() |
There was a problem hiding this comment.
Add test also with a perfect detector (Detector.pnr())
tests/test_processor.py
Outdated
| @@ -32,7 +32,7 @@ | |||
| from unittest.mock import patch | |||
|
|
|||
| import perceval as pcvl | |||
| from perceval import BSDistribution | |||
| from perceval import BSDistribution, Herald, BSSamples | |||
There was a problem hiding this comment.
Why new import in this file ?
Aubaert
deleted the
PCVL-846-incompatible-heralds-and-detectors-should-not-be-simulated
branch
Draft PR for now since the test with
probswill only work after merging with #515 that removes a normalization that currently raises a warning.