Conversation
…systems based on MCMC-simulations.
wowtor
left a comment
wowtor
left a comment
There was a problem hiding this comment.
Deze PR moet het makkelijker maken om de bounds voor meerdere systemen in eens te berekenen.
Ik mis denk ik wat context. Hoe zou dit toegepast moeten worden in een pipeline? En is dit alleen voor MCMC relevant of ook andere systemen? Zo nee, is dit onnodig ingrijpend voor alle andere systemen? Misschien is het eenvoudiger om het systeem binnen MCMC op te lossen. Of een parallelle set bounders op te zetten die het allemaal in een keer doen en dat ook efficient kunnen. Wat betreft de implementatie vind ik dat er veel impliciete semantiek in zit, wat code complexer en foutgevoeliger maakt.
| self.lower_llr_bound = np.ones(llrs.shape[1]) * np.nan | ||
| self.upper_llr_bound = np.ones(llrs.shape[1]) * np.nan |
There was a problem hiding this comment.
| self.lower_llr_bound = np.ones(llrs.shape[1]) * np.nan | |
| self.upper_llr_bound = np.ones(llrs.shape[1]) * np.nan | |
| self.lower_llr_bound = np.empty(llrs.shape[1]) | |
| self.upper_llr_bound = np.empty(llrs.shape[1]) |
| def calculate_bounds(self, llrs: np.ndarray, labels: np.ndarray) -> tuple[float | None, float | None]: | ||
| def calculate_bounds( | ||
| self, llrs: np.ndarray, labels: np.ndarray | ||
| ) -> tuple[float | np.ndarray | None, float | np.ndarray | None]: |
There was a problem hiding this comment.
Dit is een abstract method die een andere return type geeft. Alle subklassen moeten dit weten en aangepast worden zodat ze hier mee om kunnen gaan. Is dat echt nodig?
|
Ik heb nu geprobeerd het op een andere manier in NetherlandsForensicInstitute/lr_module_scratch#15 op te lossen. Lijkt dat jouw een betere aanvliegroute? |
Ik zou voor die route kiezen inderdaad, omdat ik denk dat het eenvoudiger is, en beter te begrijpen. |
|
Via de andere route geimplementeerd |
2 participants
Add support for multiple LR-systems during bounding, required for LR-systems based on MCMC-simulations.