New functionality for bounding LR systems #108
Conversation
…I. Alberink et al. that is being submitted for publication.
There was a problem hiding this comment.
Om te beginnen, super dat jullie al een PR hebben voor deze functionaliteit, en goed om dit in LIR te hebben.
Wat nog ontbreekt is een manier om het in een LIR pipeline te passen. Je kan dat doen door er een calibrator van te maken, met een fit() en transform() en zo. Zie bijvoorbeeld ELUBbounder. Daar kan hij naast staan. Hoe noemen jullie deze manier van bounding?
| @property | ||
| def p0(self): | ||
| return self.first_step_calibrator.p0 | ||
| if self.also_fit_calibrator: |
There was a problem hiding this comment.
Deze attribute also_fit_calibrator is gedefinieerd binnen de super class, maar wordt alleen in de sub class gebruikt. Dat is niet overzichtelijk. Er zijn meerdere opties:
- de attribute verplaatsen naar de sub class
- deze code verplaatsen naar de super class, en dan hier:
super().fit(X, y) - deze method
fit()helemaal naar de super class en daar ook een abstract methodcalculate_bounds()toevoegen die dan hier geimplementeerd wordt
Die laatste vind ik denk ik het mooist omdat dat het duidelijkst de scheiding aanbrengt in verantwoordelijkheden tussen de super class en de sub class.
| def fit(self, X, y): | ||
| """ | ||
| assuming that y=1 corresponds to Hp, y=0 to Hd | ||
| """ | ||
| if self.also_fit_calibrator: | ||
| self.first_step_calibrator.fit(X,y) | ||
| lrs = self.first_step_calibrator.transform(X) | ||
| self.first_step_calibrator.fit(X, y) |
There was a problem hiding this comment.
zelfde comment als bij de andere bounder
| def p1(self): | ||
| return self.first_step_calibrator.p1 | ||
| y = np.asarray(y).squeeze() | ||
| self._lower_lr_bound, self._upper_lr_bound = calculate_invariance_bounds(lrs, y)[:2] |
There was a problem hiding this comment.
Dit is een goede oplossing. Een andere optie om aan te geven dat je de laatste twee waarden niet wil gebruiken is dit:
self._lower_lr_bound, self._upper_lr_bound, _, _ = calculate_invariance_bounds(lrs, y)
Based on a novel publication by I. Alberink et al. that is being submitted for publication.