Conversation
…vschneller/metagen
… into davschneller/metagen
…o davschneller/metagen
vikaskurapati
left a comment
There was a problem hiding this comment.
Mostly looks okay to me apart from some queries. Could you please address them?
| outfiles = [] | ||
| for gendata in self.generators: | ||
| outdirname = f'metagen_{gendata["name"]}' | ||
| outdir = os.path.join(outputDir, outdirname) |
There was a problem hiding this comment.
Where is outputDir defined in this scope? If it is intended to be a global variable, can we please avoid that and pass it as a function argument?
There was a problem hiding this comment.
Whoops; missed that one. Made it a function arg.
yateto/metagen.py
Outdated
| args = gendata['args'] | ||
| kwargs = gendata['kwargs'] | ||
|
|
||
| fixArchitectureGlobal(generator._arch) |
There was a problem hiding this comment.
Can we use a getter function from Generator class instead of accessing the so called private variable here? I know this won't break anything, but it may be a cleaner design.
There was a problem hiding this comment.
Yes, we can (I agree, it's better that way). Done in the upcoming commit.
| splitname = prename.split('::') | ||
|
|
||
| def inner(): | ||
| name = splitname[-1] |
There was a problem hiding this comment.
How are we guaranteeing that splitname is never an empty list here? I see Line 122 has splitname = prename.split('::'). So we could either have a check that prename is always consistently formatted, or if splitname is never empty. You could choose which is a better check.
There was a problem hiding this comment.
That actually shouldn't happen; since even without a '::' we'll just get a list with a single element, it being the whole string.
Looking it up for certainty (cf. here https://docs.python.org/3.6/library/stdtypes.html#str.split ): since we use split( ) with an argument, it doesn't happen. But it could if we had split(). I can add a comment for that.
There was a problem hiding this comment.
What happens if prename is an empty string? Or is it someone we can guarantee would never happen with absolute certainty? I still think it is better to have such checks in the function, so it is self-contained. But if you think it is too redundant, I would yield here.
There was a problem hiding this comment.
If you mean about the split, then: prename = '' => prename.split('::') == [''] .
(i.e. we just get one element which is the whole string)
(can probably be confirmed in any Python console around if needed)
| def compile_list(self, outputDir=''): | ||
| outfiles = [] | ||
| for gendata in self.generators: | ||
| outdirname = f'metagen_{gendata["name"]}' |
There was a problem hiding this comment.
Not a review comment, but a general query: how do you choose which variable name to be camel case, and which variable to be normal like this? For example: outdirname is not camelcase, but the function argument outputDir is camelcase. Is it that all internal variables are not camel cases, and then class members, and function arguments are camel cases?
There was a problem hiding this comment.
Let's say... It's a bit arbitrary. The outputDir being camel case is more a relic since all of Yateto uses that convention quite a bit; especially in the older commits it seems.
And so, outputDir is usually in camel case within Yateto for now.
Usually, per the official Python style guide, snake case is the "official" way to name things; though snake case can sometimes appear a bit hard to read IMO (it just gives you less to focus on—visually at least).
... that made me think; at some point we might wanna apply the whole usual swath of Python linters on Yateto. I've started setting up a pre-commit in #109.
Add a way to call Yateto multiple times in a row with different configurations and template the kernels and tensors by call.
Subroutine caching needs to be done externally still (cf. #80 ; can be done with the datastructures introduced there).
Can be e.g. used to disambiguate kernels by precision or even more (like, e.g. convergence order or material class).
Needed for SeisSol/SeisSol#1421 and maybe others to come.