Open fink-stanislav opened 7 months ago
Thanks @fink-stanislav for posting this issue and suggesting fixes.
is_trained
status, but we must ensure it has no side effects elsewhere. Basically, this status serves as a double check during the conformalization process: the attribute train
associated with an instance of ConformalPredictor
(defined in module deel.puncc.api.conformalization
) needs to be consistent with the training status of the underlying predictor. I will look further into that. predictor.is_trained
is an iterable, the test predictor.is_trained is False
is indeed inconsistent. Using an attribute getter, as you suggest, looks like a great solution 👍 !We will keep you updated on the progress regarding these issues. Thanks again for your findings.
Hi @fink-stanislav
We have fixed the issue that you reported. However, regarding your first point, switching to tuples would cause a break in the current API. Therefore, I will reopen this issue and consider it an enhancement for the future version 0.8.x.
Thank you for your valuable contribution, and please let us know if you have any further concerns or suggestions.
Module
Prediction (API)
Contact Details
stanislau.fink@gmail.com
Current Behavior
DualPredictor
is supposed to be initialized withis_trained
parameter that should be a list of booleans. There are a few issues related to this variable:is_trained
is not updated after methodfit
is completed:Instead, you may want to rewrite it like this:
is_trained
is a part of condition ofif
statement:In
elif
statementpredictor.is_trained
is used as boolean but in fact it can be a list if predictor is an instance ofDualPredictor
. In this case it will beTrue
if the list is not empty even though a model can still be not trained.The solution suggested in point 2 is only partial. I think the best way would be to keep
is_trained
variable private (rename it to_is_trained
) and introduce a propertyis_trained
which will behave as instance variable but will be implemented as a method under the hood. For example forDualPredictor
it can look like this:Expected Behavior
is_trained
is expected to be booleanis_trained
is expected to change after models are trainedVersion
v0.9
Environment
No response
Relevant log output
No response
To Reproduce
I do not have example of a code that actually fails because of that. The test named
test_locally_adaptive_cp
actually runs the part of the code with aforementionedelif
statement.