misc: Add deviceid to configuration and enhance switchconfig #2175
Conversation
devito/types/parallel.py
Outdated
| @@ -252,7 +252,7 @@ class DeviceID(DeviceSymbol): | |||
|
|
|||
| @property | |||
| def default_value(self): | |||
| return -1 | |||
| return configuration['deviceid'] | |||
There was a problem hiding this comment.
We can return a default_value based on global state
I think the better way to go here would be overriding _arg_values (from Scalar)
Within the override, we check, in order:
- whether kwargs has a user-override for
deviceid - whether
configuration['deviceid']is !=default_value - else return
default_value
devito/parameters.py
Outdated
| @@ -7,6 +7,7 @@ | |||
| from devito.logger import info, warning | |||
| from devito.tools import Signer, filter_ordered | |||
|
|
|||
|
|
|||
devito/operator/operator.py
Outdated
| @@ -1186,7 +1186,9 @@ def parse_kwargs(**kwargs): | |||
|
|
|||
| # `allocator` | |||
| kwargs['allocator'] = default_allocator( | |||
| '%s.%s' % (kwargs['compiler'].__class__.__name__, kwargs['language']) | |||
| '%s.%s.%s' % (kwargs['compiler'].__class__.__name__, | |||
There was a problem hiding this comment.
Can we just make a name property in our Compilers so we don't have to do this
| self.previous = {} | ||
| for k, v in self.params.items(): | ||
| self.previous[k] = configuration[k] | ||
| configuration[k] = v |
There was a problem hiding this comment.
self.previous = self.params.copy()
configuration.update(**self.params)
There was a problem hiding this comment.
this has a slightly different effect, although I could write it more succinctly:
self.previous = {k: configuration[k] for k in self.params.keys()}
configuration.update(**self.params)
There was a problem hiding this comment.
actually, configuration.update(**self.params) doesn't work, since the update method has a different meaning in configuration
devito/parameters.py
Outdated
| try: | ||
| configuration[k] = self.previous[k].name | ||
| except AttributeError: | ||
| super(Parameters, configuration).__setitem__(k, self.previous[k]) |
There was a problem hiding this comment.
How can we end up here? isn't this line 247?
There was a problem hiding this comment.
Not exactly, this bypasses the @_check_key_value decorator and the _update_functions callback. The value we're setting back into the configuration has already gone through those and in some cases (e.g. compiler and platform) it has been transformed. So the previous checks and transformations need to be skipped.
| elif configuration['deviceid'] != self.default_value: | ||
| return {self.name: configuration['deviceid']} | ||
| else: | ||
| return {self.name: self.default_value} |
There was a problem hiding this comment.
why not just {self.name: kwargs.pop(self.name, self.default_value)} and have default_value return configuration['deviceid']
There was a problem hiding this comment.
isn't that what the original code did? I thought the idea was to keep default_value and configuration['deviceid'] separate?
There was a problem hiding this comment.
yes, agree with Carlos here. We can't just return a (mutable) global value from default_value :-)
Codecov Report
@@ Coverage Diff @@
## master #2175 +/- ##
==========================================
- Coverage 87.10% 87.09% -0.01%
==========================================
Files 226 226
Lines 40148 40160 +12
Branches 7325 7329 +4
==========================================
+ Hits 34969 34979 +10
- Misses 4600 4602 +2
Partials 579 579
|
Adds
deviceidtoconfigurationand updatesswitchconfigso that it can act both as a decorator and a context manager.