EIA client #109
EIA client #109
Conversation
There was a problem hiding this comment.
@andydevlinsmith Thanks for this great PR. I have provided some feedback in comments that I hope is helpful. Let me know if anything doesn't make sense. The test suite fails on JAE for me, did you have that issue?
pyiso/__init__.py
Outdated
| 'DOPD': {'class': 'EIACLIENT', 'module': 'eia_esod'}, | ||
| 'DUK': {'class': 'EIACLIENT', 'module': 'eia_esod'}, | ||
| 'EEI': {'class': 'EIACLIENT', 'module': 'eia_esod'}, | ||
| 'ELE': {'class': 'SVERIClient', 'module': 'sveri'}, |
There was a problem hiding this comment.
Kudos for following our existing naming conventions, but I think this is starting to get confusing. Maybe a better way would be to just have an EIA client that took the name of the balancing authority as an argument to the methods? Or had a set_BA function? @marcpare thoughts?
There was a problem hiding this comment.
Added a set_BA function to the class.
pyiso/eia_esod.py
Outdated
| LOGGER.error('No end_at date provided') | ||
| raise ValueError('You must specify an end_at date.') | ||
|
|
||
| def handle_options(self, **kwargs): |
There was a problem hiding this comment.
Most of this is done by base.handle_options that can be called with super(EIAClient, self).handle_options(**kwargs). Take a look at caiso.handle_options for how we have handled this in the past. If base.handle_options doesn't do everything, you can add the extra handling in here.
There was a problem hiding this comment.
Ok, I think I have this fairly well trimmed down now.
| @@ -0,0 +1,645 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Thanks for a great test suite! Comments and answers to some of your questions below
There was a problem hiding this comment.
@ndavis6, unless I'm overlooking something I think I've addressed all the comments here. Please let me know if you see any other issues, particularly with the set_ba function I added to eia_esod.py. Thanks.
tests/test_eia.py
Outdated
| # return | ||
| return data | ||
|
|
||
| # this is repeated- super it? |
There was a problem hiding this comment.
Super it. Super all of them. You could merge these two tests with one extra argument. It looks like the differences are the set of keys ['load_MW', ... vs ['ba_name',... and the keys 'load_MW' and 'net_exp_MW' when checking for numeric value.
There was a problem hiding this comment.
Done for _run_test and _run_null_response_test. I'll see if there are others I can combine to clean things up a bit.
There was a problem hiding this comment.
Combined a few more tests here.
tests/test_eia.py
Outdated
| data = c.get_trade(**kwargs) | ||
|
|
||
| # test number | ||
| self.assertGreaterEqual(len(data), 1) |
There was a problem hiding this comment.
I would add a msg='BA is %s' % ba_name to the assert arguments. If you add ``self.longMessage = TrueinTestEIA.setUp``` it adds this message to the original assertion message, That let's the user know which BA is failing (or at least which is the first BA to fail), so that it is clear if a single BA is causing all the failures.
pyiso/eia_esod.py
Outdated
|
|
||
| if self.options['data'] == 'gen': | ||
| if self.options['forecast']: | ||
| LOGGER.error('Forecast generation data error for %s' % self.NAME) |
There was a problem hiding this comment.
I would change Logger.error message here and line 221 to "forecast not supported for generation' to match the msg in ValueError.
pyiso/eia_esod.py
Outdated
| from pyiso import LOGGER | ||
|
|
||
|
|
||
| class EIACLIENT(BaseClient): |
There was a problem hiding this comment.
Minor, but we only capitalize the first letter of non-acronyms (EIAClient)
|
Many thanks for the comments! I'll get these in and update the PR. |
This addresses #97.
This is my first PR for this project, so constructive feedback would be welcome.
Caveats:
Thanks!