Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update parameter to be savable #1518
Update parameter to be savable #1518
Changes from all commits
ef226c5
423e69a
72f2a34
42bda31
aff13c2
bfb9574
ce72133
26398c5
726ab54
36a1176
36f1564
9b1c620
46a93ac
eebdfa3
e8852cf
1ccaa72
f435db6
7c24d72
8c82cdd
ff53817
33f88d3
700eb46
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is mainly because of legacy, but why are we still using the term
transform
? We could make the interface cleaner if we followed TFP's naming and used the wordbijector
instead. Having two words for the same object makes the code harder to read and understand. This PR seems like the right place and time to update this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like Bijector name, because not every bijector that TFP has is actually a bijection. Transform name has been used since the start of gpflow and I think keeping that name indeed reflects meaning better. And another reason for transform - keep the commit diff minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The super class already has this implemented. Making use of it makes it more clear what is happening:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're comment is right, the suggestion is wrong (I guess you know that though) - If you're going to do that, you can simply remove the method, it'll inherit it from the parent.
I suppose what you're suggesting is
Which makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, the transformation will be performed 2 times. We redefine the method so that we could do extra steps before assigning value without sacrificing performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what you mean. The snippet @sam-willis posted seems correct to me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will only be performed twice if you call
_validate_unconstrained_value
, which is why I suggested just usingtf.debugging.assert_all_finite
. You could make the message a constant, if you don't want code duplication.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove
_validate_unconstrained_value
,_unconstrained_value
and_constrained_value
, and just check that after either assignment or initialisation the unconstrained value is finite.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't apply Sam's suggestion because it changes the value first and then checks correctness, whereas it should be vice versa.
Also, I don't understand what you mean by:
I think you can remove _validate_unconstrained_value, _unconstrained_value and _constrained_value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I do see your point, maybe it's better to not change the value if it's invalid, although an exception is thrown, so I'm not sure how important it is. I guess if you catch the exception your current code makes recovering easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_constrained_value
is unused, and technically you should always have the identity for the transform now, so you don't need theif
in_unconstrained_value
, but I think those are very minor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't TF2.2 resolve the issue with cast - can we simply use tf.case now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still support 2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not add a
ref
/deref
method toParameter
? Seems like this is just missing from TFP'sTransformedVariable
API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter is not a tensor object and not a trait to the tensor class, it simply a wrapper to a tensor. You cannot use the same reference for two different objects. That was one of the reasons why saving didn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be cleaner to have
in order to get rid of
ref_fn
andderef_fn
? It seems to me that this referencing functionality could be handy elsewhere as well at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ref()
belongs to tensor API, andref()
returns a different type. There must be a distinction between these two for clarity. I would prefer not to introduce this method to the parameter class because of a printing issue.