-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify class names #29
Comments
For the first question, I suggest something like : class AgeReplacementPolicy:
def __init__(self, model: LifetimeModel[*TupleArrays]):
self.model = model
self.policy = None
self.fitted = False
def load(self, /, *args, nature: str = "default", **kwargs):
if self.policy is not None:
raise ValueError("AgeReplacementPolicy is already load")
elif nature == "default":
self.policy = AgeReplacementPolicy(self.model, *args, **kwargs)
elif nature == "one_cycle":
self.policy = OneCycleAgeReplacementPolicy(self.model, *args, **kwargs)
elif nature == "poisson":
self.policy = NHPPAgeReplacementPolicy(self.model, *args, **kwargs)
else:
raise ValueError(f"Uncorrect nature {nature}")
def load_fit(self, /, *args, nature: str = "default", **kwargs):
if self.policy is not None:
raise ValueError("AgeReplacementPolicy is already load")
elif nature == "default":
self.policy = AgeReplacementPolicy(self.model, *args, **kwargs).fit()
self.fitted = True
elif nature == "one_cycle":
self.policy = OneCycleAgeReplacementPolicy(
self.model, *args, **kwargs
).fit()
self.fitted = True
elif nature == "poisson":
self.policy = NHPPAgeReplacementPolicy(self.model, *args, **kwargs).fit()
self.fitted = True
else:
raise ValueError(f"Uncorrect nature {nature}")
def fit(self):
if self.fitted:
raise ValueError("AgeReplacementPolicy is already fitted")
self.policy = self.policy.fit()
def __getattr__(self, item):
class_name = type(self).__name__
if self.policy is None:
raise ValueError("No policy as been loaded")
if item in super().__getattribute__("policy"):
return super().__getattribute__("policy")[item]
raise AttributeError(f"{class_name} has no attribute/method {item}") This implementation provides a unified factory class to manage all age replacement policies. The Example usage would be something like : >>> weibull = Weibull(...)
>>> gompertz = Gompertz(...)
>>> policy = AgeReplacementPolicy(model)
>>> policy.load_fit(cf = , cp =, ...) # default
>>> policy.expected_total_cost(timeline)
>>> another_policy = AgeReplacementPolicy(gompertz)
>>> another_policy.load_fit(nature="poisson", cf = , cr = , ...) or maybe pass the I think that the big downside of this approach is the difficulty in documenting the arguments that can be passed to the |
To relate it to #32, In addition, I would prefer using a common argument named For other arguments, I would also remove At the moment, |
While working on branch v2.0.0/nhpp, I find it cumbersome to differentiate between
OneCycleAgeReplacementPolicy
,AgeReplacementPolicy
, andNHPPAgeReplacementPolicy
, as they are all, in essence, age replacement policies.First question:
Why not create a factory to redirect to the correct policy instantiation depending on the passed arguments?
I can see two possible approaches:
Create a factory within the
__new__
method of a singleAgeReplacementPolicy
class. This could look like:policy = AgeReplacementPolicy(..., one_cycle=True)
policy = AgeReplacementPolicy(..., nhpp=True)
This approach is similar to what I proposed in More user-friendly way to construct LeftTruncatedModel, etc. #27
Create a factory function,
age_replacement_policy
, which would return one ofOneCycleAgeReplacementPolicy
,AgeReplacementPolicy
, orNHPPAgeReplacementPolicy
.Second question:
Why not rename
AgeReplacementPolicy
toAgeReplacement
?I believe the term "policy" could be implicit here, resulting in a more concise class name for the user. Names like
NHPPAgeReplacementPolicy
andOneCycleAgeReplacementPolicy
are too long, making them harder to remember and prone to typos.I do acknowledge that one might raise concerns about potential ambiguity between
AgeReplacementModel
andAgeReplacement
. However, I don’t think this would be an issue sinceAgeReplacementModel
is an internal class and isn’t directly exposed through the user API.The text was updated successfully, but these errors were encountered: