Skip to content
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

Expose params to znrnd.models classes as parameter #52

Open
jhossbach opened this issue Sep 8, 2022 · 2 comments
Open

Expose params to znrnd.models classes as parameter #52

jhossbach opened this issue Sep 8, 2022 · 2 comments

Comments

@jhossbach
Copy link
Collaborator

Instead of writing something like network.model_state.params we can add a property to the class for convenience

@property
def params(self):
    return self.model_state.params

This is just a tracker for a future PR until #49 is merged

@KonstiNik
Copy link
Member

The idea of the train state is that it includes everything you need for training.
I think it is reasonable to get to the parameters through train state only, because using params as an additional attribute might seem confusing then.
I actually would prefer it how it is right now.

@SamTov
Copy link
Member

SamTov commented Sep 9, 2022

I don't really mind either way, I would suggest that if you make the param then you need to be absolutely sure that it gets updated automatically when the model state is updated, i.e that it is a pointer, which I do not know off the top of my head. If you can ascertain this and you can make that work nicely then I would say do as you please. The params are fundamental even if the train state goes which for other models it might.

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

No branches or pull requests

3 participants