-
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
API: Revamp sparse subfunction #2374
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2374 +/- ##
==========================================
- Coverage 86.80% 78.30% -8.51%
==========================================
Files 233 226 -7
Lines 43755 43170 -585
Branches 8078 8040 -38
==========================================
- Hits 37983 33804 -4179
- Misses 5063 8608 +3545
- Partials 709 758 +49 ☔ View full report in Codecov by Sentry. |
6a5af47
to
abdee0b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2374 +/- ##
==========================================
+ Coverage 86.80% 86.82% +0.01%
==========================================
Files 233 233
Lines 43755 43770 +15
Branches 8078 8077 -1
==========================================
+ Hits 37983 38003 +20
+ Misses 5063 5060 -3
+ Partials 709 707 -2 ☔ View full report in Codecov by Sentry. |
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 is such an outstanding cleanup... !
I've left one tiny coding comment, other than that we good
break | ||
except AttributeError: | ||
continue | ||
else: |
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.
Wrong indent level for this else
?
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 the else
goes there with the for
. It's a python construct "if for didn't exit then " that avoids wrapping the for in a try/if
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.
Huh, TIL
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 and it's quite handy
if isinstance(key, SubFunction): | ||
if d in key.indices: | ||
# Can use as is, dimension already matches | ||
if self.alias: |
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 self.alias:
return key._rebuild(alias=self.alias, name=name)
return key
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 like to keep the else for simple blocks and unindent when it's a decent code chunck that is the "main" part
return key._rebuild(alias=self.alias, name=name) | ||
else: | ||
return key | ||
else: |
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 else
can be removed and the contents of the block unindented by a level.
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.
same
@@ -486,8 +486,7 @@ def interpolation_coeffs(self): | |||
sf = SubFunction(name="wsinc%s" % r.name, dtype=self.sfunction.dtype, | |||
shape=shape, dimensions=dimensions, | |||
space_order=0, alias=self.sfunction.alias, | |||
distributor=self.sfunction._distributor, | |||
parent=self.sfunction) | |||
parent=None) |
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: we can probably save a line here by compacting these four lines into three, but obviously this is for another day
No description provided.