fix type in Parameter type check

[joelberkeley-secondmind]

fixes a minor mistake in the types

[codecov]

Codecov Report

Merging #1500 into develop will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1500   +/-   ##
========================================
  Coverage    95.25%   95.25%           
========================================
  Files           82       82           
  Lines         3752     3752           
========================================
  Hits          3574     3574           
  Misses         178      178           

Impacted Files	Coverage Δ
gpflow/base.py	92.40% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9e125e...d1e5603. Read the comment docs.

[st--]

Happy with the type changes. Just wondering about the change on line 50: I would normally think re-using code is better than copying it - if you think the _IS_PARAMETER by itself is overkill, maybe just go and remove both of these everywhere?

[st--]

What was the motivation behind no longer reusing the _IS_PARAMETER defined above?

[joelberkeley-secondmind]

mypy doesn't know that _IS_PARAMETER(o) == True means o is a Parameter, and that's needed for o.trainable to type check. I think mypy special-cases isinstance so it can work it out from that

[joelberkeley-secondmind]

if it were possible, i'd also mildly prefer using _IS_PARAMETER

[st--]

Seems to me like a case where mypy is flawed and it'd be better code leaving it as it had been with a # type: ignore at the end (and maybe a comment explaining what you wrote above).

[joelberkeley-secondmind]

mypy's fine. it's a type theory thing afaict. I don't think any language would be able to prove that

[joelberkeley-secondmind]

at least not without pattern matching

[joelberkeley-secondmind]

Happy with the type changes. Just wondering about the change on line 50: I would normally think re-using code is better than copying it - if you think the _IS_PARAMETER by itself is overkill, maybe just go and remove both of these everywhere?

you mean like ...?

class Module(tf.Module):
    @property
    def parameters(self) -> Tuple["Parameter", ...]:
        return tuple(self._flatten(predicate=lambda o: isinstance(o, Parameter)))

i'm not fussed either way really