-
Notifications
You must be signed in to change notification settings - Fork 229
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
example: small cleanup of tti for easier reuse #2294
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2294 +/- ##
==========================================
- Coverage 86.76% 86.74% -0.02%
==========================================
Files 229 229
Lines 42893 42895 +2
Branches 7953 7955 +2
==========================================
- Hits 37214 37210 -4
- Misses 5004 5009 +5
- Partials 675 676 +1 ☔ View full report in Codecov by Sentry. |
392d350
to
b74393b
Compare
b74393b
to
8bdf54e
Compare
@@ -922,7 +922,7 @@ def _arg_defaults(self, _min=None, size=None, alias=None): | |||
except AttributeError: | |||
factor = dim._factor | |||
|
|||
defaults[dim.parent.max_name] = range(1, factor*size - 1) | |||
defaults[dim.parent.max_name] = range(0, factor*size - 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.
nitpicking: you can omit the 0,
part
why was there a 1 before?
is it worth adding a test?
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 think the 1 was there for any specific reason, just a "run at least one step" but not really needed
@@ -2874,7 +2874,7 @@ def test_opcounts(self, space_order, expected): | |||
|
|||
@switchconfig(profiling='advanced') | |||
@pytest.mark.parametrize('space_order,exp_ops,exp_arrays', [ | |||
(4, 122, 6), (8, 221, 7) | |||
(4, 122, 6), (8, 235, 7) |
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 the increase? variable density on by default? that feels weird
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.
No it's because of the space order I think. THe model used to be setup with space order 4
while setting up the second case with space order 8
so not sure if it was creating a few low order derivatives.
All the other opcount case stay the same the FD hasn't changed (b is never setup in oss for tti) it's just the space order 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.
Nothing to add that Fabio hasn't already mentioned
Minor cleanup for easier call from other places