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
Updated setup.cfg mypy flags and resolved related errors. #703
Updated setup.cfg mypy flags and resolved related errors. #703
Changes from 1 commit
a74ef96
8b1d198
cb58600
1eb64ae
0ad2a38
0313deb
2d5307f
82cbf42
81f9f36
c1b2bd1
3c9aaa3
1de9958
cc8224e
2538ea3
b737249
60a2202
fde3a97
481a8ec
9a261e1
06762be
25094c2
1ee80eb
a9d097b
d7ee476
841adf5
9c2d636
78f57fa
ffbcb43
e417189
ec2745b
e6399be
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.
This might be better to do a
T
generic because really the class should be original of the input? Otherwise, would this convert something using this metaclass to that type later in code?Is that why loading has the issue as well?
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 issue is that
new_class
needs to be typed asAny
initially because otherwise mypy complains that._register_subclass()
doesn't exist. I'm thinking we could also do:which does pass mypy.
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 agree that
new_class: AutoSubRegistrationMeta = ...
would not work either. However, I was wondering if there's a generic approach that could fix this that allows us to donew_class: T = ...
or some sort whereT
is thecls
or something of the like. If it is too complex, fair. Either way, it doesn't feel right to me thatcast(AutoSubRegistrationMeta, new_class)
shouldn't be somecast(T, new_class)
as the output is not a classAutoSubRegistrationMeta
, right?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.
Was able to get it working with this change (which I just committed). Couldn't get around the
._register_subclass()
line because that's done dynamically.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 believe the
Any
is required to bestr
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.
Makes sense. Made the change.
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.
mypy compains if self.match_count isn't a float?
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.
Actually, now that I look at the code again, I don't see
self.match_count
getting initialized anywhere. This would explain why it's being typed asAny
(thus requiring some sort of cast).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 fixed it by manually adding a type for it in the constructor. I assume
self.match_count
is populated in some dynamic way so mypy isn't able to pick up its type automatically.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 we convert later to a float bc this could be an np.float64. Probably good to say this could be np.float so that we are returning the real type.
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 see. This would still require a
cast
though because numpy's type annotations forsqrt
on a scalar has a return type ofAny
.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'm trying to cast it to
np.float64
but the issue becomes that there's not really a compatible return type annotation becausenp.nan
is of typefloat
. Using something likenp.float_
causes mypy to complain about thenp.nan
case and sticking withfloat
causes mypy to complain about thenp.sqrt
case.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.
Changing it to a Union causes issues with line 385:
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.
So
def stddev(self) -> Union[float, np.float64]:
with the
cast(np.float64, np.sqrt(self.variance))
doesn'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.
Yes. It seems to cause issues with other functions.
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.
Can you elaborate, we shouldn't be forcing it to fix mypy, but instead the typing should be valid. In this case, we aren't being valid by casting
np.float64
as afloat
. The reason this is problematic is that code that might try to applyjson.dumps
to this down the line would fail if it wasn't converted into a float first.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 would assume
def stddev(self) -> Union[float, np.float64]:
even w/o the cast should be functional.but if this is erroring elsewhere, then we should fix that once we do this correctly.
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.
is this a float or is it a np.float64?
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.
@Sanketh7
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.
This does seem to be a np.float64. Because biased_skewness could be either
np.nan
or somenp.float64
, I've updated any usage of biased skewness to be aUnion[float, np.float64]
.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.
as above
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.
Updated
_correct_bias_skewness
to returnUnion[float, np.float64]
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.
as above
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.
Updated
_merged_biased_kurtosis
return returnUnion[float, np.float64]
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.
is this a list of float or np.float64?
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.
bin_edges
doesn't necessarily need to contain numpy float values.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 issue is if we are prescribing it to contain something that isn't true. I'd rather say it could contains either if that is true because otherwise if someone is relying on static typing to ensure functionality of a function, this could be cause them to infer incorrectly.
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.
Done
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 did some more investigating and numpy functions end up converting the values to
float64
so this should only beList[np.float64]
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.
Actually it would be
List[float]
because.tolist()
converts values to the compatible python type.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.
as above
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.
This will end up being a np.float64. I changed the type annotation and method return type to reflect 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 think these would also be np.float64
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.
Casting this to
np.float64
and changing the return type toUnion[float, np.float64]
gives me a similar issue as one of the other methods: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.
Did google provide any response to 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.
so it has something to do with the return of these numpy funcs being any maybe?
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.
Should be fixed. See #703 (comment)
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.
this would be a float64
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'm getting a similar issue as before:
Looking into it more, I believe it's because we're trying to assign a
Union
to aTypeVar
withbound=Subtractable
and theUnion
itself does not follow theSubtractable
protocol. So far the only reasonable solution I can think of is doing:which isn't very ideal. I'll look into mypy generics more to see if there's a workaround.
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.
Fixed. See #703 (comment)
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.
if this can be any, we technically don't need union, right?
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.
However, will this error then bc we are using Any?
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 guess the Union was redundant. Changing to Any actually doesn't cause issues with our mypy flag
warn_return_any = True
because that only complains when we return Any and the function return type is not Any.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.
nice