-
Notifications
You must be signed in to change notification settings - Fork 31
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
BiG-CZ: CUAHSI Variable Values using Ulmo and WaterML #2328
Conversation
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.
Just took a look through the code — pulling it down now to test. I like the way you've set this up, and thanks for the helpful commit messages!
Comments are either clarification or thoughts on alternative ways to implement some of the pieces, all pretty optional.
params = request.query_params | ||
catalog = params.get('catalog') | ||
|
||
if not catalog: |
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 wonder if there should be some sort of request serializer that would handle all the validation here and in here. Validation errors off a serializer would also be a list, instead of the way it's done here, returning only one missing field error at a time.
This isn't that necessary at the moment, but could be something we add when we want to expose the API.
var values = this.get('values'); | ||
|
||
values.reset(response.values); | ||
mrv = response.values[response.values.length - 1].value |
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.
Is response.values
guaranteed to have at least 1 value?
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.
Also, this line is missing a semi-colon. When the build works again, you may see some bad lint output.
values.reset(response.values); | ||
mrv = response.values[response.values.length - 1].value | ||
|
||
delete response.values; |
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.
Why do we delete it?
src/mmw/js/src/data_catalog/views.js
Outdated
last_dates = variables.map(function(v) { | ||
var values = v.get('values'); | ||
|
||
return new Date(values.at(values.length - 1).get('datetime')); |
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.
Consider using values.last()
if possible.
src/mmw/js/src/data_catalog/views.js
Outdated
var mode = this.model.get('mode'), | ||
variables = this.model.get('variables'), | ||
view = mode === 'table' ? | ||
new CuahsiTableView({ collection: variables }) : |
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.
For some of our other toggles, we render all views onShow
(or wherever) and then do the switching via display: none;
styling on the non-active view (bootstrap might take care of this?).
I find the way you implemented it here a lot easier to follow, but I think the other way reduces the amount of rendering needed.
src/mmw/js/src/data_catalog/views.js
Outdated
}, | ||
|
||
fetchComplete: function() { | ||
this.model.set({ fetching: false, error: false }); |
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.
Consider moving these (fetchComplete
, fetchError
) to the model.
Thanks for taking a look. I'm going to take this feedback into account, and add some more features, and will let you know when this is ready for another look. |
Instead of list of concept_keyword strings, we now return a list of objects with keys: code, name, and concept_keyword. This will allow us to continue using concept_keyword rather than the very verbose name as the display field, and adding it under the hood to the code necessary for fetching detail values. We still serve the name, in case the full form is needed. Some sample values: { "code": "NWISUV:63680", "concept_keyword": "Turbidity", "name": "Turbidity, water, unfiltered, monochrome near infra-red LED light, 780-900 nm, detection angle 90 +-2.5 degrees, formazin nephelometric units (FNU)" } { "code": "NWISUV:00301", "concept_keyword": "Oxygen, dissolved percent of saturation", "name": "Dissolved oxygen, water, unfiltered, percent of saturation" } { "code": "NWISUV:99988", "concept_keyword": "Radiation, incoming PAR", "name": "Photosynthetically active radiation (average flux density on a horizontal surface during measurement interval), micromoles of photons per square meter per second" }
Ulmo (https://github.com/ulmo-dev/ulmo/) is a data access library designed to fetch information from various hydrology and climatology sources. It works on the WaterML standard, which is a protocol built on top of SOAP. We will use it to fetch detailed values for sensor data from CUAHSI. Also upgrade pip which speeds up our Python dependency installation step significantly.
This fetches detailed information for any given site. This should be used for the detail page in the app.
This fetches values for any given site and variable. The date range can be optionally specified, by default it is one week up to today. These values can be used for the chart and table in the detail view.
Previously we were using the same Marionette view with differing templates for each catalog. Now that we are trying to add custom events and sub-views, this approach no longer suffices. We make a base view which has open / close functionality built in, and extend it for each catalog, supplying the template in the child view. Instead of a dictionary of templates, CATALOG_RESULT_DETAILS_VIEWS now holds the corresponding view.
These describe the individual variable calls, and the returned values
Different sites and variables have values only for certain ranges. Querying for ranges outside of those lead to errors. Now we first query the /details endpoint to get the units and begin and end date for each variable, then validate the given search dates to be within those (and use sensible defaults of 1 week or entire date range, whichever is shorter, if no from and to dates are specified). Furthermore, we only search each site once. If the values are successfully retrieved, they will be shown immediately the next time the user opens the details page for that site. If the value retrieval errors out for some reason, we retry that every time the details page is opened.
d225262
to
137373e
Compare
I'm going to split this work into back-end and front-end pieces, and open two separate PRs for each. Closing this in the meantime. Thanks for your feedback! |
Overview
Fetches values from CUAHSI / WDC using Ulmo for each variable at a site. We're currently only showing the latest value in the table, with charts coming in a follow-up PR.
Connects #2243
Connects #2238
Demo
Notes
This PR adds two endpoints:
/details
and/values
. Currently we are only using/values
in the UI, but may use/details
in the future. There is a dummy placeholder for the charts, but no values.Known Issues
The popover for the headline for WDC Details View doesn't work.
Testing Instructions
app
andworker
. Run thebundle
script.