Adds acis.py for ACIS Web Services functionality #177

WEP11 · 2018-01-09T15:23:22Z

Adds acis.py, which provides a function for connecting to ACIS Web Services. This function is Python 3 compatible, but will need to use urllib2 to become Python2.7 compatible.

…rvices

Updates docstring text to better reflect parameters

dopplershift

Thanks! I have a few comments, but really they just relate to better integration into Siphon. Also, can you move this into the simplewebservice directory? That's where we're starting to accumulate things like this.

Overall, the code does seem fine and clearly would simplify access to the service. Would be good to eventually add an example to show how useful this is.

dopplershift · 2018-01-17T00:31:50Z

siphon/acis.py

+        timeout=60
+
+    try:
+        response = urllib.request.urlopen(req, timeout=timeout)  # 10-Minute Timeout


I don't think that comment is correct any more 😁

Would also need to be updated to use requests.

dopplershift · 2018-01-17T00:39:06Z

siphon/acis.py

+        return json.loads(response.read())
+    except urllib.request.HTTPError as error:
+        if error.code == 400:
+            print(error.msg)


Instead of catching the error, printing the message, and returning False, I think it's better to just let the error bubble to the top. That turns the code for users from (currently):

response = acisRequest(...) if response == False: # Handle error else: # Do stuff with data

into

try: response = acisRequest(...) # Do stuff with data except HTTPError: # Handle errors

It's shorter code in the library, just as easy for users, and doesn't involve having changing return types. If you're worried about having multiple exception types, you could create a custom Exception subclass and raise that from inside the function.

dopplershift · 2018-01-17T00:40:21Z

siphon/acis.py

+import json
+import socket
+
+def acisRequest(method, params):


How about acis_request? That matches our existing naming structure.

That certainly can be done.

dopplershift · 2018-01-17T00:41:07Z

siphon/acis.py

+
+    Returns
+    ----------
+    Connection Success: A dictionary that is derived from JSON data from the remote API


Would need to be updated based on discussion below. Also Sphinx will pitch a fit unless the length of "---------" matches the length of "Returns".

dopplershift · 2018-01-17T00:41:59Z

siphon/acis.py

+
+    Parameters
+    ----------
+    method - The Web Services request method (StnMeta, StnData, MultiStnData, etc)


Parameters docs look like:

method : str The Web Services request method (StnMeta, StnData, MultiStnData, etc.)

dopplershift · 2018-01-17T00:47:34Z

siphon/acis.py

+
+    base_url = 'http://data.rcc-acis.org/'  # ACIS Web API URL
+
+    req = urllib.request.Request(base_url+method,


Do you think you could adapt this to use the requests module? We're already using it internally, it would solve your Python 2 problem, and it sets our own "User-Agent" header. I'm happy to look into how to do this if you can clarify how the data argument gets used.

The data argument is simply a JSON object containing parameters (Station ID, dates, variables, etc) for the HTTP request. I'm more than happy to adapt it, there was no reason for using urllib other than that's what was used in the old Python 2 version of this code.

Ok, then I'll leave you to it, but feel free to scream if you run into problems.

WEP11 · 2018-01-17T16:48:46Z

I am working on some use case examples. Will those go into /examples?

dopplershift · 2018-01-17T20:04:11Z

Yeah, I think that makes sense, though if you have several, you could do examples/acis.

- Updates acis_request to use the requests library.

…Center/siphon

- Adds examples and resources for Siphon documentation

WEP11 · 2018-01-18T17:17:51Z

Latest push adds two examples to the documentation and finalizes some of the acis_request function code. Exception handling is certainly open to feedback.

dopplershift

Looks like I failed to submit this yesterday. Oops.

Exception handling actually looks fine, at least to my eyes.

Some general style comments, especially for examples, to make the bots happy:

a space after every comma
Only use single quotes
a space after the colon in a dictionary

Otherwise, I think these examples look great!

dopplershift · 2018-01-17T23:39:58Z

siphon/simplewebservice/acis.py

+    base_url = 'http://data.rcc-acis.org/'  # ACIS Web API URL
+
+    if method == 'MultiStnData':
+        timeout=300


Could use some more spaces around the = here. Or if you like:

timeout = 300 if method == 'MultiStnData' else 60

but that's just a matter of personal taste.

dopplershift · 2018-01-17T23:40:39Z

siphon/simplewebservice/acis.py

+        timeout=60
+
+    try:
+        response = requests.post(base_url+method, data=params, timeout=timeout)


Can you put some spaces around the +?

Also, if you change requests.post() to create_http_session().post() (after doing from ..http_util import create_http_session), you'll get e.g. "Siphon(0.6.0)" in the logs for ACIS. Not sure if you'd be interested in seeing how much use the service gets from us--or if you even have access to those logs yourself. This one really doesn't matter to me since we'll never see the logs.

dopplershift · 2018-01-17T23:41:19Z

siphon/simplewebservice/acis.py

+    Parameters
+    ----------
+    method : str
+        The Web Services request method (MultiStn,StnMeta,etc)


Is the exhaustive list of methods up above? If so, probably good to just list all 5 here rather than abbreviating the list.

dopplershift · 2018-01-18T22:33:53Z

siphon/simplewebservice/acis.py

+
+class ACIS_API_Exception(Exception):
+    """
+


Can you kill the blank line?

dopplershift · 2018-01-18T22:34:08Z

siphon/simplewebservice/acis.py

+        timeout=60
+
+    try:
+        response = requests.post(base_url+method, json=params, timeout=timeout)


Can you add spaces around the +?

Also, if you change requests.post() to create_http_session().post() (after doing from ..http_util import create_http_session), you'll get e.g. "Siphon(0.6.0)" in the logs for ACIS. Not sure if you'd be interested in seeing how much use the service gets from us--or if you even have access to those logs yourself. This one really doesn't matter to me since we'll never see the logs.

Neat! I'll have to double check with the developer that manages the servers to make sure, but I know it'd be interesting to see. (We're more than happy to share those statistics if we get them)

dopplershift · 2018-01-18T22:40:58Z

examples/acis/Mapping_Example.py

+# all 4 days of data, we would simply remove that parameter.
+#
+# To wrap up this example, we will finally plot our precipitation sums and
+# departures onto a map using CartoPy and MetPy. To do this we will utilize


It doesn't look like you're actually using MetPy here.

You caught me! I was, but then I noticed the implementation I had was poor. Looks like I forgot to proofread that part of the tutorial afterwards.

dopplershift · 2018-01-18T22:45:09Z

siphon/simplewebservice/acis.py

+    except requests.exceptions.TooManyRedirects:
+        raise ACIS_API_Exception("Bad URL. Check your ACIS connection method string.")
+    except ValueError:
+        raise ACIS_API_Exception("No data returned! The ACIS parameter dictionary\


Single quotes, please. Also, instead of the line continuation (\), you can just end the line with a ', and then open a new string with a ' on the next line; since it's within the (), Python will concatenate them for you. So:

raise ACIS_API_Exception('No data returned! The ACIS parameter dictionary' 'may be incorrectly formatted')

dopplershift · 2018-01-18T22:49:32Z

We'll also need to add a test. Probably easiest to start with one from siphon/tests/test_wyoming.py. The recorder.use_cassette the first time you run the test will record all of the web requests and responses; future runs of the test (when it can find the recorded data--so it needs to be included in a commit) will use the recorded data so that we don't have tests that need a proper network, server availability, etc. You'll need to make sure vcrpy is installed.

If setting that up is a problem, I'm happy to jump in and help out.

- Changes use of requests directly in acis_request() to use siphon create_http_session() function.

WEP11 · 2018-01-19T15:53:21Z

Alright, I have a unit test file that runs fine when run I run the individual function. How can I run the test to generate the recorded data you mentioned? Thanks!

WEP11 · 2018-01-19T19:30:41Z

This version adds the unit test, fixes the formatting errors mentioned, and adds the unit test data to the fixtures folder. I completely missed that the data was already present in my fork (I imagined a gitignore for some reason). I'll monitor the bots to see what they find, thanks!

dopplershift

Just a minor change on the test, otherwise that looks just like what I was looking for. Modulo some minor style things, this is almost there.

dopplershift · 2018-01-19T19:55:32Z

siphon/tests/test_acis.py

+recorder = get_recorder(__file__)
+
+@recorder.use_cassette('acis_request')
+def test_acis():


Can you break this up into 4 individual tests, one for each request method?

I certainly can

WEP11 · 2018-01-19T21:19:53Z

siphon/tests/test_acis.py

+    """Testing ACIS Gridded Data request."""
+    data = acis_request('GridData', {'loc': '-95.36, 29.76', 'sdate': '2000-01',
+                        'edate': '2000-07', 'grid': '3', 'elems': [
+                        {'name': 'maxt', 'interval': 'mly', 'reduce': 'max',


I'm unsure of how to take care of this PEP8 error. I thought line 44 might have been too long, but instead I created the E128 error.
siphon/tests/test_acis.py:44:25: E122 continuation line missing indentation or outdented siphon/tests/test_acis.py:45:25: E128 continuation line under-indented for visual indent

Try this:

data = acis_request('GridData', {'loc': '-95.36, 29.76', 'sdate': '2000-01', 'edate': '2000-07', 'grid': '3', 'elems': [{'name': 'maxt', 'interval': 'mly', 'reduce': 'max', 'smry': 'max'}]})

Arguments to the function, keys in the dictionary, items in the list--they line up when wrapped.

That seems to have worked on my end, we'll see if that silences the Travis-CI. (Right after I make the acis_request function in acis.py have an imperative mood)

WEP11 · 2018-01-19T21:20:56Z

siphon/tests/test_acis.py

+recorder = get_recorder(__file__)
+
+
+@recorder.use_cassette('acis_request')


I used one "cassette" for all 5 tests.. Is that alright? They're all JSON strings.

I think that's fine for now. If we ever need to regenerate, we'll have to separate them, since the cassette is only ever generated from a single test.

dopplershift

Found a few things, but nothing I’d hold this up for. Feel free to ignore. Let me know and I’ll merge.

dopplershift · 2018-01-29T23:55:10Z

examples/acis/Mapping_Example.py

+
+for stn in myData['data']:
+    # Skip threaded stations! They have no lat/lons
+    if stn['meta']['sids'][-1][-1] == '9':


Maybe

if stn[‘meta’][‘sids’][-1].endswith(‘9’)

dopplershift · 2018-01-29T23:55:15Z

examples/acis/Basic_Overview.py

+avgt = []
+dates = []
+for obs in myData['data']:
+    if obs[0][-2:] == '01':


How about:

If obs[0].endswith(‘01’)

?

dopplershift · 2018-01-29T23:56:39Z

examples/acis/Mapping_Example.py

+    if stn['meta']['sids'][-1][-1] == '9':
+        continue
+    # Skip stations with missing data
+    if stn['smry'][0] == 'M' or stn['smry'][1] == 'M':


What comes before “M” if it’s at index 1? White space? If so, what about:

If stn[‘smry’].lstrip().startswith(‘M’)

?

In this case 'smry' is actually a list of summary values. So in this case the 'smry' would have one value for the total precip and another for the departure from normal.

WEP11 · 2018-01-30T14:33:55Z

That push should take care of most of the changes you found.. I 'dismissed' your review I guess?

dopplershift · 2018-01-30T19:42:09Z

I have it set to clear the review when new code is uploaded.

This is good. Thanks! 🎉 Congrats on your PR!

Adds acis.py, which provides a function for connecting to ACIS Web Se…

795a439

…rvices

WEP11 requested review from dopplershift, jrleeman and lesserwhirls as code owners January 9, 2018 15:23

Update acis.py

e5df7d1

Updates docstring text to better reflect parameters

dopplershift requested changes Jan 17, 2018

View reviewed changes

WEP11 added 4 commits January 17, 2018 15:06

- Moves acis.py to the simplewebservice directory.

90d52dd

- Updates acis_request to use the requests library.

Merge branch 'master' of https://github.com/HighPlainsRegionalClimate…

6a23567

…Center/siphon

Fixes syntax error in acis_request

5ef7f3a

- Finalizes acis_request function with requests

bea518c

- Adds examples and resources for Siphon documentation

Fixes issues raised in Codacy check

5a5a44c

dopplershift reviewed Jan 18, 2018

View reviewed changes

- Fixes formatting issues

f333710

- Changes use of requests directly in acis_request() to use siphon create_http_session() function.

WEP11 added 2 commits January 19, 2018 11:38

Adds unit test for acis.py

7d00b16

Adds acis_request VCR data to siphon/tests/fixtures

1bb8b4b

dopplershift requested changes Jan 19, 2018

View reviewed changes

WEP11 added 4 commits January 19, 2018 14:04

Splits acis_test into 5 separate unit tests.

43a51e3

Adds neccessary comments to each unit test in test_acis.py

17860de

Fixes initial style issues found by Travis-CI

c62483f

Fixes more PEP8 style issues found by Travis-CI

67227f4

WEP11 commented Jan 19, 2018

View reviewed changes

WEP11 added 2 commits January 19, 2018 15:38

Fixes PEP-8 style issues found by Travis-CI

fd8bee4

Merge branch 'master' into master

4dee3bc

dopplershift self-assigned this Jan 28, 2018

dopplershift added this to the 0.7 milestone Jan 28, 2018

dopplershift previously approved these changes Jan 30, 2018

View reviewed changes

Makes Pythonic changes to some string searches.

fab1c2a

WEP11 dismissed dopplershift’s stale review via fab1c2a January 30, 2018 14:31

dopplershift approved these changes Jan 30, 2018

View reviewed changes

dopplershift merged commit c664e04 into Unidata:master Jan 30, 2018


		base_url = 'http://data.rcc-acis.org/' # ACIS Web API URL

		req = urllib.request.Request(base_url+method,

		recorder = get_recorder(__file__)


		@recorder.use_cassette('acis_request')

Adds acis.py for ACIS Web Services functionality #177

Adds acis.py for ACIS Web Services functionality #177

Conversation

WEP11 commented Jan 9, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WEP11 commented Jan 17, 2018

dopplershift commented Jan 17, 2018

WEP11 commented Jan 18, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift commented Jan 18, 2018

WEP11 commented Jan 19, 2018

WEP11 commented Jan 19, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WEP11 commented Jan 30, 2018

dopplershift commented Jan 30, 2018