Skip to content

Conversation

@thijssnelleman
Copy link
Collaborator

#412

This is a proposed solution to this issue; Hyperparameters have properties that are not updated by changing the values. By adapting the updating functionality, it first tests if there already is a value present, and if so, verifies what needs to be recalculated.

  1. Assumes that in the initialisation all values are set exactly once, otherwise the update will be triggered and may cause issues when properties are accessed that do not exist yet
  2. Shares a lot of code with that found in the init function. Would be better if this would be settled in one function rather than spread over two
  3. Needs to be adapted for each HP type. Need to find out if we can do this more generally (For example, what can be done in the parent Hyperparameter class? Not everything but maybe something like default value?)

Have written a partial pytest to verify the behaviour, seems to work just fine with this suggested solution

Copy link
Contributor

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a lot easier to just reconstruct the whole hyperparameter, and then steal it's values. This means you don't have to duplicate validation, and you can use this method with minor adjustments on everywhere.

Read up on dataclasses. There is a method signature .replace() -> Self which will basically do this, but it returns a new instance.

Just take special note that we define a custom __init__ for dataclasses, which is not normally what you should do. This means you don't get all their benefits. They exist to be backwards compatbile with the public API

def __setattr__(self, name: str, value: Any):
    init_params: dict[str, Any] = # get init params
    if name not in init_params:
        raise ValueError("Can't set attribute {name}, must be one passed to init.") # Something better error message than this
    init_params[name] = value
    
    # This raises if invalid
    new_instance = self.__class__(**init_params)
    self.__dict__ = new_instance.__dict__ # Unsure if this works exactly

@thijssnelleman
Copy link
Collaborator Author

That is actually a good idea, although it could be inefficient in some cases. I went with a slightly different strategy; the setattr has now a different workflow if a value is not being set from init. The check is quite ugly, but works. Instead of using replace, I am just calling self.init again. Although current tests are basics, they are passing, wondering what @eddiebergman thinks of this solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants