Skip to content
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

Aggregate network objects #607

Merged
merged 46 commits into from
Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
af8b6c9
Initial commit
OwlHute Apr 10, 2019
da6a9ec
Fix spelling of AggregateNetworkObjects in aggregate_network_objects.py
OwlHute Apr 10, 2019
e3709c1
Merge branch 'master' into aggregate_network_objects
greenape Apr 11, 2019
0362d40
Recommit with review comments addressed & integration_tests all passing
OwlHute Apr 11, 2019
fdabdcb
Merge branch 'aggregate_network_objects' of https://github.com/Flowmi…
OwlHute Apr 11, 2019
5bee2a9
Merge from master
OwlHute Apr 11, 2019
2ffd1d0
Integrate master including Spatial Aggregate enhancement
OwlHute Apr 12, 2019
7515587
Commit with corrected arguments
OwlHute Apr 12, 2019
ead7c62
Commit with corrected arguments
OwlHute Apr 12, 2019
abafc78
Amend approved schemas.txt file
OwlHute Apr 12, 2019
ff17cf1
Fix error within exception handler if payload isn't present.
Apr 12, 2019
08c392c
Rename variable
Apr 12, 2019
e406b34
Merge branch 'master' into aggregate_network_objects
maxalbert Apr 12, 2019
711485b
Catch TypeError earlier and return meaningful error message.
Apr 12, 2019
300b73c
Change 'by' to 'period' in aggregate_network_objects
OwlHute Apr 13, 2019
032ed58
update approved.txt for 'by' to 'period' change
OwlHute Apr 13, 2019
358b92b
Commit to complete merge from master
OwlHute Apr 14, 2019
93fec24
Merge branch 'master' into aggregate_network_objects
greenape Apr 15, 2019
7851ba7
Rework with consistent use of parameters total_by & aggregate_by
OwlHute Apr 15, 2019
4dc986e
Merging Pipfile.lock changes
OwlHute Apr 15, 2019
818c009
In total_network_objects.py change 'default' directive to 'missing'
OwlHute Apr 15, 2019
cf63cd0
In total_network_objects.py change 'default' directive to 'missing'
OwlHute Apr 15, 2019
4653d1b
Fix approved.txt for total_by default value (missing='day') & JSON er…
OwlHute Apr 15, 2019
7e87031
Fix another expected JSON error message
OwlHute Apr 15, 2019
5c6cd3b
Fix buglet in test_total_network_objects.py
OwlHute Apr 15, 2019
655a067
Merge branch 'master' into aggregate_network_objects
greenape Apr 15, 2019
28e21f6
Remove validation argument suppliable to custom fields, to increase t…
OwlHute Apr 15, 2019
0d26c19
Merge from master
OwlHute Apr 15, 2019
49f2378
complete merge
OwlHute Apr 15, 2019
a9f6129
Merge branch 'master' into aggregate_network_objects
greenape Apr 16, 2019
deea38e
Add a test for the typeerror catch in the run query action handler
greenape Apr 16, 2019
4a12e7a
Lint
greenape Apr 16, 2019
1632cd8
Implement review changes of 2019-04-16 am
OwlHute Apr 16, 2019
48e05f0
Commit merge changes from origin/aggregate_network_objects
OwlHute Apr 16, 2019
cf1664e
In CHANGELOG.md added comment re new time aggregation parameter names
OwlHute Apr 16, 2019
a41060f
In CHANGELOG.md move comment re new time aggregation parameter names …
OwlHute Apr 16, 2019
9a97e6d
Fix link in CHANGELOG.md
OwlHute Apr 16, 2019
e9116bb
Move note of TotalNetworkObjects amendment from 'Added' section to 'C…
OwlHute Apr 16, 2019
3f723b4
Tweak changelog
greenape Apr 16, 2019
9a1689e
Indent
greenape Apr 16, 2019
4bab4ed
Fix changelog
greenape Apr 16, 2019
9204ede
Merge branch 'master' into aggregate_network_objects
greenape Apr 17, 2019
c1f4cfe
Merge branch 'master' into aggregate_network_objects
greenape Apr 17, 2019
1d4c2bd
Update approved specs
greenape Apr 17, 2019
a0b7212
Merge branch 'master' into aggregate_network_objects
greenape Apr 17, 2019
79d179e
Hit missed codepaths in client
greenape Apr 17, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## [Unreleased]
### Added
- [#601] Implemented new flowclient API entrypoint, aggregate_network_objects(), to access (with simplified parameters) equivalent flowmachine query

- [#581] Implemented new flowclient API entrypoint, total_network_objects(), to access (with simplified parameters) equivalent flowmachine query

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a changed entry here as well to reflect that the names of the arguments to TotalNetworkObjects and AggregateNetworkObjects have been altered?

greenape marked this conversation as resolved.
Show resolved Hide resolved
- [#577] Implemented new flowclient API entrypoint, location_introversion(), to access (with simplified parameters) equivalent flowmachine query
Expand Down
4 changes: 4 additions & 0 deletions docs/notebook_preamble.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ def format_dict(x):
"permissions": {"run": True, "poll": True, "get_result": True},
"spatial_aggregation": ["admin3", "admin2", "admin1"],
},
"aggregate_network_objects": {
"permissions": {"run": True, "poll": True, "get_result": True},
"spatial_aggregation": ["admin3", "admin2", "admin1"],
},
}

TOKEN = make_token(
Expand Down
4 changes: 4 additions & 0 deletions docs/source/4-developer.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ The API exposes four routes:

At present, the following query types are accessible through FlowAPI:

- `aggregate_network_objects`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


Statistics about unique cells/sites aggregated to a period.

- `daily_location`

A statistic representing where subscribers are on a given day, aggregated to a spatial unit.
Expand Down
1 change: 1 addition & 0 deletions flowauth/backend/flowauth/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ def make_demodata(): # pragma: no cover
"unique_subscriber_counts",
"location_introversion",
"total_network_objects",
"aggregate_network_objects",
):
c = Capability(name=c)
db.session.add(c)
Expand Down
2 changes: 2 additions & 0 deletions flowclient/flowclient/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
unique_subscriber_counts,
location_introversion,
total_network_objects,
aggregate_network_objects,
)

__all__ = [
Expand All @@ -54,4 +55,5 @@
"unique_subscriber_counts",
"location_introversion",
"total_network_objects",
"aggregate_network_objects",
]
28 changes: 28 additions & 0 deletions flowclient/flowclient/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1022,3 +1022,31 @@ def total_network_objects(
"end_date": end_date,
"aggregation_unit": aggregation_unit,
}


def aggregate_network_objects(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been giving the params for this some thought - I actually think what would be most useful is to have it take: network_objects, statistic, by. That can be achieved without touching the underlying Flowmachine class, by constructing a tno object, and then calling aggregate on it with the by and statistic arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to expand on that a bit, in terms of roughly how the call would look. I gather tno is TotalNetworkObject, and "by" is possibly the date range?! But also, I thought you were trying to do away with calls to aggregate() (#599)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you were, I understand now what you are driving at! (in view of the review comment further down)

Copy link
Member

@greenape greenape Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguments need to be total_network_objects_query: dict, statistic and by: str (or a more explanatory name than by would also be fine!)

start_date: str, end_date: str, aggregation_unit: str
) -> dict:
"""
Return query spec for aggregate network objects

Parameters
----------
start_date : str
ISO format date of the first day of the count, e.g. "2016-01-01"
end_date : str
ISO format date of the day _after_ the final date of the count, e.g. "2016-01-08"
aggregation_unit : str
Unit of aggregation, e.g. "admin3"

Returns
-------
dict
Dict which functions as the query specification
"""
return {
"query_kind": "aggregate_network_objects",
"start_date": start_date,
"end_date": end_date,
"aggregation_unit": aggregation_unit,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

from marshmallow import Schema, fields, post_load
from marshmallow.validate import OneOf, Length

from flowmachine.features import AggregateNetworkObjects
from .base_exposed_query import BaseExposedQuery
from .custom_fields import AggregationUnit

__all__ = ["AggregateNetworkObjectsSchema", "AggregateNetworkObjectsExposed"]


class AggregateNetworkObjectsSchema(Schema):

start_date = fields.Date(required=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to have the right fields, one of which is a bit tricky because it should refer to another schema. See FlowsSchema, or ModalLocationSchema for an example of how to do that.

end_date = fields.Date(required=True)
aggregation_unit = AggregationUnit()

@post_load
def make_query_object(self, params):
return AggregateNetworkObjectsExposed(**params)


class AggregateNetworkObjectsExposed(BaseExposedQuery):
def __init__(self, *, start_date, end_date, aggregation_unit):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to match the AggregateNetworkObjects form now

# Note: all input parameters need to be defined as attributes on `self`
# so that marshmallow can serialise the object correctly.
self.start_date = start_date
self.end_date = end_date
self.aggregation_unit = aggregation_unit

@property
def _flowmachine_query_obj(self):
"""
Return the underlying flowmachine aggregate_network_objects object.

Returns
-------
Query
"""
return AggregateNetworkObjects(
maxalbert marked this conversation as resolved.
Show resolved Hide resolved
start=self.start_date, stop=self.end_date, level=self.aggregation_unit
)
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@
LocationIntroversionExposed,
)
from .total_network_objects import TotalNetworkObjectsSchema, TotalNetworkObjectsExposed
from .aggregate_network_objects import (
AggregateNetworkObjectsSchema,
AggregateNetworkObjectsExposed,
)
from .dfs_metric_total_amount import (
DFSTotalMetricAmountSchema,
DFSTotalMetricAmountExposed,
Expand All @@ -48,6 +52,7 @@ class FlowmachineQuerySchema(OneOfSchema):
"unique_subscriber_counts": UniqueSubscriberCountsSchema,
"location_introversion": LocationIntroversionSchema,
"total_network_objects": TotalNetworkObjectsSchema,
"aggregate_network_objects": AggregateNetworkObjectsSchema,
"dfs_metric_total_amount": DFSTotalMetricAmountSchema,
}

Expand Down Expand Up @@ -76,6 +81,8 @@ def get_obj_type(self, obj):
return "location_introversion"
elif isinstance(obj, TotalNetworkObjectsExposed):
return "total_network_objects"
elif isinstance(obj, AggregateNetworkObjectsExposed):
return "aggregate_network_objects"
elif isinstance(obj, DFSTotalMetricAmountExposed):
return "dfs_metric_total_amount"
else:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def aggregate(self, by=None, statistic="avg"):
)


class AggregateNetworkObjects(GeoDataMixin, Query):
class AggregateNetworkObjects(TotalNetworkObjects, Query):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relates to your comment in #601 "Refactor AggregateNetworkObjects to take TotalNetworkObjects as main argument"

Copy link
Member

@greenape greenape Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I think I've failed to communicate what I meant there - the suggestion was that rather than taking all the arguments necessary to create a TotalNetworkObjects object in the __init__ method, that this class should instead just take a TotalNetworkObjects object instead of those arguments.

This change actually alters the class hierarchy of the class (https://www.digitalocean.com/community/tutorials/understanding-class-inheritance-in-python-3 is a decent overview of class inheritance in python), and I imagine will cause a few issues because TotalNetworkObjects also inherits from Query, which would give you a somewhat odd inheritance hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll amend the code accordingly. (I do understand python class hierarchies, but I'll have a quick skim of that page)

"""
Class for calculating statistics about unique cells/sites
and aggregate it by period.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def test_get_available_queries(send_zmq_message_and_receive_reply):
"unique_subscriber_counts",
"location_introversion",
"total_network_objects",
"aggregate_network_objects",
"dfs_metric_total_amount",
]
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
"FlowmachineQuerySchema": {
"discriminator": {
"mapping": {
"aggregate_network_objects": "#/components/schemas/AggregateNetworkObjects",
"daily_location": "#/components/schemas/DailyLocation",
"dfs_metric_total_amount": "#/components/schemas/DFSTotalMetricAmount",
"dummy_query": "#/components/schemas/DummyQuery",
Expand All @@ -104,6 +105,9 @@
"propertyName": "query_kind"
},
"oneOf": [
{
"$ref": "#/components/schemas/AggregateNetworkObjects"
},
{
"$ref": "#/components/schemas/DFSTotalMetricAmount"
},
Expand Down Expand Up @@ -145,6 +149,33 @@
}
]
},
"AggregateNetworkObjects": {
"properties": {
"aggregation_unit": {
"enum": [
"admin0",
"admin1",
"admin2",
"admin3"
],
"type": "string"
},
"end_date": {
"format": "date",
"type": "string"
},
"start_date": {
"format": "date",
"type": "string"
}
},
"required": [
"aggregation_unit",
"end_date",
"start_date"
],
"type": "object"
},
"Flows": {
"properties": {
"aggregation_unit": {
Expand Down
8 changes: 8 additions & 0 deletions integration_tests/tests/test_queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@
"direction": "in",
},
),
(
"aggregate_network_objects",
{
"start_date": "2016-01-01",
"end_date": "2016-01-02",
"aggregation_unit": "admin3",
},
),
# (
# # TODO: currently flowclient.modal_location() doesn't accept a 'locations'
# # argument but rather expects a list of location objects. We can't test this
Expand Down