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

Change TransformState to NamedTuple #106

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Conversation

SamDuffield
Copy link
Contributor

This is quite a large PR unfortunately but fixes #83 and also cleans up the TransformState code by moving from mutable dataclass to immutable NamedTuple which is also better encapsulation practice for functional code.

One thing that's nice is that NamedTuple is already added to the pytree registry for optree and torch (fixing #83), so we don't need to do that manually as before.

There was an issue with the aux handling as modifying aux is not possible to do in-place as we don't know the structure of aux before log_posterior is called, also aux is not guaranteed to be a TensorTree and could contain strings etc.

The proposed fix is to return a new state with all other attributes modified in-place (i.e. pointers to old state) but aux replaced.

Copy link
Contributor

@KaelanDt KaelanDt left a comment

Choose a reason for hiding this comment

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

Looks good to me, but in my opinion it would be good to check how this affects memory consumption and allocation - since you allocate a new state whenever you update it, which may be quite inefficient

posteriors/types.py Outdated Show resolved Hide resolved
posteriors/laplace/dense_fisher.py Show resolved Hide resolved
@SamDuffield
Copy link
Contributor Author

Looks good to me, but in my opinion it would be good to check how this affects memory consumption and allocation - since you allocate a new state whenever you update it, which may be quite inefficient

This PR shouldn't affect memory consumption, it just changes the handling of the algorithm states to a better convention.

It would be good to have some numerics on memory consumption and the pros and cons of using inplace and even whether we should continue to support it.

There is also an element of horses-for-courses since for MCMC-style where you want to collect samples along a trajectory you need inplace=False whereas for optimization or deep ensemble-style where you only care about the final result you may prefer inplace=True if its faster (which I'm not sure on).

@KaelanDt
Copy link
Contributor

Looks good to me, but in my opinion it would be good to check how this affects memory consumption and allocation - since you allocate a new state whenever you update it, which may be quite inefficient

This PR shouldn't affect memory consumption, it just changes the handling of the algorithm states to a better convention.

It would be good to have some numerics on memory consumption and the pros and cons of using inplace and even whether we should continue to support it.

There is also an element of horses-for-courses since for MCMC-style where you want to collect samples along a trajectory you need inplace=False whereas for optimization or deep ensemble-style where you only care about the final result you may prefer inplace=True if its faster (which I'm not sure on).

I'm not sure about this: the previous inplace behaviour would change the elements of a previously allocated object. Now, you re-allocate a new object every time

Copy link
Contributor

@KaelanDt KaelanDt left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from a docstring to change

@SamDuffield
Copy link
Contributor Author

SamDuffield commented Jul 24, 2024

Looks good to me, but in my opinion it would be good to check how this affects memory consumption and allocation - since you allocate a new state whenever you update it, which may be quite inefficient

This PR shouldn't affect memory consumption, it just changes the handling of the algorithm states to a better convention.
It would be good to have some numerics on memory consumption and the pros and cons of using inplace and even whether we should continue to support it.
There is also an element of horses-for-courses since for MCMC-style where you want to collect samples along a trajectory you need inplace=False whereas for optimization or deep ensemble-style where you only care about the final result you may prefer inplace=True if its faster (which I'm not sure on).

I'm not sure about this: the previous inplace behaviour would change the elements of a previously allocated object. Now, you re-allocate a new object every time

We define a new NamedTuple but if inplace=True all Tensors are pointers to the same memory as the previous ones (aside from aux which is not guaranteed to be a TensorTree). This is also checked in the tests.

@SamDuffield SamDuffield merged commit e08e729 into main Jul 24, 2024
2 checks passed
@SamDuffield SamDuffield deleted the named-tuple-transform-state branch July 24, 2024 10:57
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.

TransformState doesn't work with torch.vmap
2 participants