Arrays as parameters #34
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 96.94% 97.07% +0.13%
==========================================
Files 12 12
Lines 1704 1849 +145
==========================================
+ Hits 1652 1795 +143
- Misses 52 54 +2
Continue to review full report at Codecov.
|
| for i, row in enumerate(v): | ||
| for j, col in enumerate(row): |
There was a problem hiding this comment.
Maybe could use np.ndindex for iterating through v?
There was a problem hiding this comment.
Nice, I didn't know about this. Not sure if I think it would be an improvement in this case though. 🤔
There was a problem hiding this comment.
Also came across it just now! The improvement for me would be readability (could be subjective), as there are many nested blocks around here and also it would be helpful to see that indeed the loops are used for indexing only
There was a problem hiding this comment.
👍 for np.ndindex, it's quite neat :)
There was a problem hiding this comment.
Hi @thisac, this looks overall nice to me! 💯 🙂 Left suggestions & questions, mostly aiming at code quality. I think the tests should mostly cover the use case that we'd like to have
There was a problem hiding this comment.
@thisac this is looking much better now I think! The serialization test is also awesome.
My only comments were with respect to the p\d variables; probably better if the change in behaviour was restricted to only tdm programs, to avoid potentially breaking existing GBS blackbird scripts
| array_insert += 1 | ||
| options = "" | ||
| # add target and type to the script | ||
| for name, data in [("target", self.target), ("type", self.programtype)]: |
There was a problem hiding this comment.
I found it difficult at first to parse this loop, may be worth checking how the code would look by adding target & type sequentially? Imho, a couple of lines duplicated lines could be fine
There was a problem hiding this comment.
Would it be nicer to instead add a separate function e.g. self.add_metadata("target") and self.add_metadata("type")?
There was a problem hiding this comment.
Looking at it again, I actually think I prefer this solution; without adding it sequentially (or creating separate functions).
There was a problem hiding this comment.
Looks awesome @thisac! 🥇 🚀 🙂 Most of the thoughts from me are smaller and related to readability. One thing I was wondering about related to the test that has a script, is if target would be missing.
Maybe something for the PR regarding docs: adding a note on which parameter names are reserved for TDM can be nice.
|
|
||
| # line break | ||
| script.append("") | ||
|
|
||
| # add variables to the script |
There was a problem hiding this comment.
Personally, would be in for separating parts of this method into multiple smaller methods (e.g., here having this as add_variables). (Commenting here, as this part of the method is newly added in this PR.)
There was a problem hiding this comment.
I fully agree with this. 💯 I feel like this is the case for quite a few parts of the Blackbird code, so actually a refactor, that simply splits many parts of the code into sub-functions, would be really nice. Maybe this can be added separately as a refactor PR some time? What do you think @josh146?
There was a problem hiding this comment.
I agree! Definitely something that would be great to improve
| # add variables to the script | ||
| if self.programtype["name"] == "tdm": | ||
| from .listener import NUMPY_TYPES # pylint:disable=import-outside-toplevel | ||
| inv_type_map = {np.dtype(v).kind: k for k, v in NUMPY_TYPES.items()} |
There was a problem hiding this comment.
(Minor suggestions:) this seems to be something that could be generally used, how about having it in listener.py (e.g., after the def of NUMPY_TYPES)?
| for k, v in self._var.items(): | ||
| var_type = inv_type_map[np.array(v).dtype.kind] | ||
| array_string = "" | ||
| if isinstance(v, np.ndarray): | ||
| for row in v: | ||
| array_string += "\n " + "".join("{}, ".format(i) for i in row)[:-2] | ||
| script.append("{} array {} ={}".format(var_type, k, array_string)) | ||
| else: | ||
| script.append("{} array {} ={}".format(var_type, k, v)) |
| kwargs.append("{}={}".format(k, v)) | ||
| else: | ||
| kwargs.append('{}="{}"'.format(k, v)) |
There was a problem hiding this comment.
(Minor suggestion): the difference between the two expressions is minor. It could help the reader if the value was extracted somehow to show the difference (not sure if this the suggestion is much better though 😅)
if self.programtype["name"] == "tdm" and v[0] == "p" and v[1:].isdigit():
value = "{}".format(v)
else:
value = '"{}"'.format(v)
kwargs.append("{}={}".format(k, value))There was a problem hiding this comment.
Thanks @antalszava! I might just keep this as it is now. I think it's clear enough, although I see your point. 🙂
| expected = dedent( | ||
| """\ | ||
| name tdm | ||
| version 1.0 |
There was a problem hiding this comment.
Would we need a target field too here?
There was a problem hiding this comment.
Target isn't mandatory in a Blackbird script, so it's not needed (but wouldn't change anything).
There was a problem hiding this comment.
Could we have it in here? Just as a sanity check, as most TDM bb scripts would have it
|
|
||
| This release contains contributions from (in alphabetical order): | ||
|
|
||
| Theodor Isacsson |
There was a problem hiding this comment.
The changelog is looking great 😍
Co-authored-by: Josh Izaac <[email protected]>
| # if a p-type parameter is used in a tdm program, it would be considered | ||
| # a free parameter (stored in `_PAR`) and thus shouldn't be set with a | ||
| # value during expression evaluation; although, since it always has an | ||
| # accompanying variable it also needs to be stored in `_VAR` with its | ||
| # corresponding value |
Arrays cannot currently be written as parameters in a template, which is needed for the TDM experiments.
What's been changed?
int number = {par})With the changes in this PR, it will be possible to write something like the following:
Potential issues
There's no validation done on the variables/arrays when assigning values to template parameters. Thus, it is possible to assign e.g. a float to
{par}inint number = {par}, since the type validation is only done at parsing, and the template parameter assignment is done separately, after the parsing, without knowing the type.