Remove reference to deleted constant and slight pandas refactor #365
Conversation
powersimdata/scenario/scenario.py
Outdated
| @@ -98,14 +97,12 @@ def _set_status(self): | |||
| :raises Exception: if scenario not found in execute list on server. | |||
| """ | |||
| execute_table = self._execute_list_manager.get_execute_table() | |||
There was a problem hiding this comment.
It seems that the CsvStore class is dedicated to reading the ScenarioList/ExecuteList. So I guess the _parse_csv method could return a pandas.DataFrame with id as index by doing l.45:
table = pd.read_csv(file_object, index_col=0) or table = pd.read_csv(file_object, index_col="id")
That way we don't have to set_index later
| @@ -43,6 +43,7 @@ def _parse_csv(self, file_object): | |||
| :return: (*pandas.DataFrame*) -- the specified file as a data frame. | |||
| """ | |||
| table = pd.read_csv(file_object) | |||
| table.set_index("id", inplace=True, drop=False) | |||
There was a problem hiding this comment.
Any reason for not dropping the column?
There was a problem hiding this comment.
I was trying to get it to work without, but the only thing we need it for is returning scenario info as an ordered dict. If we drop the column we'll get an error doing self.info["id"] within a scenario instance, since DataFrame.to_dict only includes columns when the kind is "records"
There was a problem hiding this comment.
I see... We could drop it here and use reset_index() l. 142 of scenario_list.py:
return scenario.reset_index().to_dict("records", into=OrderedDict)[0]
There was a problem hiding this comment.
I do like that a bit better, however it results in the id column being an int type, so at some point downstream it fails during string concatenation. I'll give this some more thought, probably next year.
There was a problem hiding this comment.
What about return scenario.reset_index().astype({"id": "str"}).to_dict("records", into=OrderedDict)[0]?
There was a problem hiding this comment.
And/or can the string concatenation include an extra str() wrap around the id, so that it has less strict assumptions about its input?
There was a problem hiding this comment.
I added Ben's suggestion in the latest commit. Thinking the strict assumption about types is actually a good thing - makes things simpler for any code using the scenario object. Having it guaranteed to be an int would be fine too, just minimizing changes here. This is kinda reminiscent of Postel's law, which is basically be "flexible on inputs but not outputs" (my interpretation).
| :return: (*str*) -- scenario status | ||
| """ | ||
| try: | ||
| table = self.get_execute_table() |
There was a problem hiding this comment.
Are we expecting a KeyError from this call, or would this be better outside the try?
Purpose
I was playing around with querying the execute list and noticed we still had a reference to
server_setup.EXECUTE_LISTwhich no longer exists. Changed the exception to reflect that but also tweaked the query since it seems a little more readable.Edit: based on feedback, I expanded the scope to set the index within
CsvStoreand took the opportunity to move query logic to the relevant data access classes. I think this simplifies the scenario class, making it less concerned with parsing the csv files.What the code is doing
Nothing new
Testing
Loaded an existing scenario works, and if I delete the entry from the execute list I get the exception raised here.
Time estimate
3 min