-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add subclass to get electrification profiles and versions #616
Conversation
base_name = f"{end_use}_{tech}_" | ||
matching = [f for f in _fs.listdir(".") if base_name in f] | ||
|
||
return [f.replace(base_name, "").replace(".csv", "") for f in matching] |
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.
Note: I changed lstrip
and rstrip
to replace
here because it turns out those strip any of the given characters starting from one side until one doesn't match, so it could lead to unpredictable results.
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.
What if we have 'demand'
or 'solar'
or whatever elsewhere within the string, it seems that will get replaced as well. Should we do this via regex instead? E.g.
>>> import re
>>> s = "ab_cd_ef"
>>> match = re.search("^(?P<prefix>ab_)?(?P<main>[^_]+)(?P<suffix>_ef)?$", s)
>>> match.group('prefix')
'ab_'
>>> match.group('main')
'cd'
>>> match.group('suffix')
'_ef'
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.
@danielolsen Will that be filtered by kind
first? I think it depends on the folder structure we have, unless we simply put all profiles in one folder. In general, using string parsing methods is more readable and easier to maintain.
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.
String parsing methods are definitely clearer, but have the potential for weird side effects if the strings are not always constructed in the way that we expect.
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.
With this implementation, kind
is either "building" or "transportation". So the profiles in each of those folders will have the structure [end_use]_[tech]_[version].csv
.
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.
Agree. I will defer to @jon-hagg's decision at this point.
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 think since we are responsible for profile names and version, and in this case replacing the entire prefix up to the version, it's unlikely we run into issues. E.g. the version should hopefully not contain ".csv" or "res_cooking_standard_heat_pump". I do like regex, but since the naming is so consistent we probably don't need it at this point?
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.
Does this code operate on the local folder as well, where users may add their own arbitrarily-named profiles?
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.
It does operate on the local folder. I guess we should document the expected structure for custom profiles if we don't have that yet. My assumption is that it's not supposed to work for arbitrary names, just custom versions.
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 trust your judgement. It's not like we have a ton of users out there adding a bunch of weirdly named files yet.
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.
LGTM
Purpose
Enable loading electrification profiles from blob storage or local disk, and listing the available versions. This is kind of a draft since we haven't actually uploaded the profiles or determined if this is the best way to organize them, but it is easy to change that later if we want.
What the code is doing
New
InputBase
subclass to reuse some of the functionality (e.g. caching) but rather than overloading thescenario_info
parameter inget_data
I just created a separate method with the relevant parameters. Also, to reuse the chunk of code inDataAccess
that combines blob and local profiles, the logic is factored out into a function specific to each module and passed as a callback that takes just the filesystem object as an argument.Testing
New unit test.
Time estimate
15 min