-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
[SolutionArray] Add array size methods #1284
[SolutionArray] Add array size methods #1284
Conversation
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 addition! But I do have a question about the documenting style (other than the apparent typing issue, which should be easily resolved)
157c510
to
e65b634
Compare
|
||
.. versionadded: 3.0 | ||
""" | ||
return math.prod(self.shape) |
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.
@speth @ischoegl The reason the tests are failing for Python 3.7 is that math.prod
was added in 3.8, see: https://docs.python.org/3/library/math.html#math.prod
I suppose I'll switch it to NumPy. The reason I chose math.prod
was that the NumPy docs for ndarray.size
specify that a standard arbitrary size Python int
is returned from ndarray.size
, similar to the result from math.prod
, as opposed to np.prod
that returns an np.int_
type. It seems unlikely that this will be a problem, but when trying to match an API, I think it's useful to match it as closely as possible.
The real question here is, is this worth dropping 3.7 support over? Thoughts are welcome 😄
Edit: Although, the math
docs say that the functions return a float, unless otherwise specified, so this may not be as meaningful as I thought. I'll just switch to NumPy, never mind!
e65b634
to
af51475
Compare
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 all looks good to me.
Changes proposed in this pull request
SolutionArray
to have it better match the NumPy API.If applicable, fill in the issue number this pull request is fixing
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage