aghalarp
commented
6 years ago React-Leaflet looks like the way to go for this. Well maintained and good documentation.
Closed philipmjohnson closed 6 years ago
aghalarp
commented
6 years ago React-Leaflet looks like the way to go for this. Well maintained and good documentation.
aghalarp
commented
6 years ago The other issue is location lookup. This can be taken care of with a geocoding/geosearching plugin - and fortunately for us, there is a plethora of leaflet plugins available for this. This one seems like a particularly solid choice: Leaflet-Geosearch
Testing out the demo app, the geosearching capabilities seem quite versatile, as it can retrieve lat-lng data for just a single zipcode, as well as accurate lat-lng for full addresses (which we don't need for now, but still good to know!).
philipmjohnson
commented
6 years ago Built-in location lookup is probably not necessary. Please see my edited issue description.
anthonyjchriste
commented
6 years ago Here's an open source list of every zip code and its lat/lngs https://gist.github.com/erichurst/7882666. I say we import it into mongo, index the zipcode, and have instant lookups in mongo as it will basically be one big hash table.
I can do that if anyone is interested.
philipmjohnson
commented
6 years ago For converting a US zip code to lat/long for use by the map library, I like the idea of a mongo collection. Super simple.
For providing fine-grained lat/long data (building-level), I like the idea of an external public website.
philipmjohnson
commented
6 years ago Turns out there are 33K zip codes. I just downloaded the gist and checked how many lines were in the file. I guess we'll need a Meteor method to get the lat-long for a zip code.
philipmjohnson
commented
6 years ago Future work on this component will be done as part of #118.
philipmjohnson
commented
6 years ago Great work on this component, David! Here are some enhancement requests for better integration with the system:
Yes, please go ahead and add an optional initialBox prop to the Measurements and Events components to indicate the box to display. That will make your button on the page more helpful.
Please remove the button that takes the user to the editBox page from the BoxMap component. The reason is that only admins can edit box info, and I definitely want users to see the BoxMap component. (Making it conditionally appear when the user is the admin role complicates the interface and doesn't add enough benefit.)
Most importantly, please switch from using the Measurement collection to the SystemStats collection for obtaining current voltage, frequency, and THD data. I know that the UI will be a little less cool as a result, but I expect that the BoxMap component will be very popular and I am very concerned about the additional load on both clients and servers from this amount of data. Consider what happens when there are 500 deployed boxes and the component is thus requesting and processing Measurement data from all of those boxes. I very much doubt the component (or even the server) could handle that. Furthermore, there's really no use case where a regular user needs to know real-time PQ information for every single box simultaneously. A more reasonable scenario is the desire to know real-time info for a single box, and we have the Live Measurement component to provide that. By switching from Measurements to SystemStats, the component should be able to scale to lots of boxes and remain responsive without any usability impact.
Although this has not been clearly documented yet, it should not be possible for two boxes to occupy the same location (conceptually, the lat/long should be precise enough to identify the wall outlet). So, filtering based on location is not necessary. Filtering based on region is excellent.
Remove use of ZipCode Collection. (See comment below).
I will try to do a code walkthrough in the next few days and if I have any comments, I will add them here. This feedback is just based on UI review. Note these are very minor corrections; overall you did an amazing job!
philipmjohnson
commented
6 years ago Upon further reflection, the simplest and most efficient solution is for BoxMap to subscribe to the SystemStats collection. This collection updates a single document every 10 seconds, and the box_trend_stats field will provide the most recent Trend document for all defined boxes. What's great about this solution is that the subscription load on the client is basically constant regardless of the number of boxes, and we can move the latency up and down by adjusting the cron job underlying SystemStats. (I've edited the above comment to refer to SystemStats.)
philipmjohnson
commented
6 years ago As I documented the System Stats collection in the data model chapter (https://openpowerquality.org/docs/cloud-datamodel.html#system-stats), I realized that it doesn't yet provide the right kind of information for BoxMap. I suggest you go ahead and fix that as part of issue-69. I suggest you simply edit System Stats Collection to add a new field to the document called, say, "latest_box_trends", which would be an array of the most recent Trend document associated with each defined Box.
Then you can subscribe to the System Stats collection and get the most recent min, max, or average voltage (or frequency, or THD). It will only update once a minute, but that seems reasonable.
If you really wanted to make sure the numbers change every 10 seconds, then you could provide a field called "latest_box_measurements", which is just like the above, only it has an array of the most recent Measurement documents, not Trend documents. The overhead is no different.
philipmjohnson
commented
6 years ago I was about the delete the ZipCode collection from the view api/ directory and decided to do a search to see if anyone was using it. It appears that it's used in BoxMap. I think it would be good to take it out. If the users want to associate locations with zip codes, they can just define a region. But I don't see any purpose in calculating zip codes based on lat/long values for OPQ View. I think this is an opportunity to make the code more simple without any negative user impact.
aghalarp
commented
6 years ago Thanks for the feedback! I do have a question/thought regarding Regions:
Would there be any benefit in defining an enumeration of allowed ‘region types’, and then adding a ‘regionType’ field to the Region collection? This seems like it would be a potentially useful addition, as it would allow us to query a subset of Region documents based on a type, be it ’state’, ‘city’, ‘zipcode’, ‘building’, perhaps even ‘power grid’ or ‘substation’. Since Regions are just an abstract representation of a set of Locations, we now have a very versatile way to organize our Locations. Being able to query on a region type would then allow us to aggregate our OPQBox data in a new way, essentially giving us the ability to group boxes by a certain type of location granularity. I can see this being useful in OPQView, and my gut feeling is that it could also be potentially useful for analytics/research in OPQMauka/Makai. More data the merrier, right?
In terms of usefulness for OPQView, and more specifically the Box Map component, what this means is that we can allow the user to change the ‘Box Location Granularity’ (one way to put it), and the box markers will change positions on the map based on the selected Region Type.
What would be the benefit of re-positioning boxes on the map? It’s related to the Leaflet marker clustering plugin (https://github.com/Leaflet/Leaflet.markercluster). Since clusters calculate the average live box data value of all child markers contained within it, by clustering all boxes into their respective regions (be it state, zipcode, substation, building, etc.), we can have a wide variety of ways to organize and monitor box data. I think it could be another interesting way to interact with the Box Map.
The code is basically already implemented on master. See the ‘Data Display’ button on the top-right corner of the map, and change the ‘Box Location Granularity’ from ‘Exact Location’ to ‘Region’. The ‘Exact Location’ option positions boxes by their location coordinates, while the ‘Region’ option positions boxes based on their region coordinates. And that brings me to the one caveat - in order for this to be useful, we would need to also associate a lat-lng coordinate with a region. Currently, this works because all Regions just happened to be zip code representations, and we just happened to have a handy ZipCode collection available that maps every US zipcode to its respective lat-lng coordinate.
At the end of the day, it might just not be worth the trouble for you guys. It all just depends on how valuable you all think having a ‘regionType’ field might be. Thoughts?
philipmjohnson
commented
6 years ago David: Glad to see you're back on board, even if it's only temporary! Your work is bringing up lots of useful things to discuss.
If I understand you correctly, your idea is to provide alternative "partitionings" for a given geographic area. So, for example, we could partition the island of Oahu by zip code. Or, by city. Or, by ahupua'a. Or, by substation. And so forth. The region type (zipcode, city, ahupuaa, substation) would indicate which kind of partition to use.
I think this is a cool idea in concept, but it adds nontrivial complexity. I think we should hold off until we have more evidence that we can actually take advantage of this kind of representation.
I'd like to try to simplify the UI. First, can we get rid of the "Data Display" button in the upper right corner? Instead, I propose that we put all the information either in the display associated with the box or in the popup that appears when you click on a box. Instead of showing just the voltage, and making the user click a radio button to show frequency or THD instead, why not show all of Voltage, Frequency, and THD in the display of a box?
Second, there are currently two ways to get Box Details: either by clicking the box itself or by clicking on an icon in the "box legend" area. Let's get rid of the icon approach so there's just one way.
Third, I don't think that showing the average voltage is meaningful for aggregations of boxes. Maybe just show a number indicating the number of boxes in the circle, and then the user can click on it to zoom in and see the actual values associated with individual boxes?
Related to the above, as we further develop the processing sophistication of OPQ, we will want to change the data shown with the display of a box. For example, at some point, we will want to show the "current Incident" associated with a box (if any). So, please design the box display component with the idea that it should be as easy as possible for another developer to come in and change what data is displayed and how it is formatted. Because they definitely will want to do that at some point!
It would be nice to provide some unit and/or integration tests for your code. I have just updated the documentation today with additional information about testing in OPQ View. Please take a look:
https://openpowerquality.org/docs/cloud-view.html#quality-assurance
I recently rewrote some code to make it more amenable to testing. It brings up some general design issues that I think you will find interesting. First, here's the original code:
As I note in the comment, this code was unnecessarily hard to understand, debug, and test. To improve it, I moved the code into the Trends class definition and refactored it into a set of smaller and simpler methods, each of which became amenable to unit testing. The new implementation is here:
The associated unit tests are here:
And the associated integration test is here:
It turns out that by refactoring the original monolithic Meteor Method into a set of class methods (and by judicious use of underscore), I was able to reduce the number of lines of code almost by half. (To be fair, the original nested loop implementation is a bit faster, but at this point I prefer the gains in readability and reduction in code size.)
So, the morals of this particular story are:
The more I think about it, the more I like the idea of SystemStats containing just recent Trends, not Measurements. It lowers processing overhead without any disadvantage to the user.
aghalarp
commented
6 years ago Region Representation 'Partitioning' is a good term for it. My initial thought process was more simple: I wasn't sure how useful having a single 'regionSlug' string field would be unless we could also have a way of indicating what kind/type of region this document was representing. I do understand now that the Region collection is just a many-to-many relation to the Locations collection - it's very useful in that regard. If we ever decide to expand on the Region collection in the future, perhaps consider adding a 'type' field. But I agree with you in regards to adding nontrivial complexity - hold off until we have a stronger need for this.
Simplifying the UI I pretty much agree with your assessment here. I should mention that the initial reasoning behind only displaying one box measurement type at a time (voltage, frequency, thd), was that I was thinking ahead about adding heatmap overlays, or other kinds of data-driven map overlays. I remember chatting with Anthony or Serge about this a long time ago, and how we could use measurements/trends to generate a live heatmap of our box data. There are all sorts of interesting plugins within the Leaflet ecosystem - and I was hoping to start exploring these once I finished working on the Box Map component.
Parameterizing the UI This is a great idea - but just to clarify on your terminology, when you say the 'display of the box', are you speaking of the box marker and its label (currently containing the box name and selected measurement?). If so, I should be able to easily refactor my code to make this configurable.
Testing I know you've been busy setting up the testing framework for OPQView and I definitely want to contribute here, so thanks for the rundown. I'll try to emulate your examples as I begin to write my own unit tests.
philipmjohnson
commented
6 years ago Just noticed that if I run OPQ View with meteor npm run start (without connecting to the public database) I get the following in my shell console:
Exception from sub box_map_measurements id XvGWXyxupXtvkcQPE TypeError: Cannot read property 'timestamp_ms' of undefined
I20180603-09:14:42.253(-10)? at Subscription._handler (imports/api/measurements/MeasurementsCollection.js:61:24)And the following in my browser console:
errorClass details undefined error "Zipcode not found."
errorType "Meteor.Error"
isClientSafe true
message "[Zipcode not found.]"
reason undefined
stack
:
"Error: [Zipcode not found.]↵ at Connection._livedata_result (http://localhost:3000/packages/ddp-client.js?hash=a547928b29e722e419948bbc25e954f2c268a1c3:1963:25)↵ Please be sure that the BoxMap component behaves "reasonably" when run in standard development mode.
aghalarp
commented
6 years ago Good catch - I've been solely relying on the public database during development and I overlooked this. For the ZipCodes collection, it looks like we'll be getting rid of that altogether - but what is your recommended/preferred approach here in general, for components that might rely on collections only available on Emilia - such as the Live Data component? Should we output an error message in the UI, or just allow the component to 'run' without any input data (such as measurements)?
philipmjohnson
commented
6 years ago The component should try to do something reasonable in the event that data it wants is not available. In the case of the boxmap component and missing measurement data, it can show the location of all known boxes on the map, but if measurement (actually Trend) data is not available, then it could show something like "Frequency: N/A" in the interface.
In many cases, the UI should not interpret the lack of data as an error. That said, I can think of certain cases in which missing data should be interpreted as an error! For example, say you retrieve the location slug for a box, and then can't find that slug in the Locations collection when you go looking for the lat/long coordinates. That should trigger an error, because it indicates an integrity problem in the database. But that should almost never happen.
aghalarp
commented
6 years ago General Changes
Parameterizing the UI I put a good amount of thought into how to best design this so that a future developer can easily make changes to the map without having to invest a lot of time into figuring out the inner-workings of Leaflet and the marker-cluster plugin that we are using.
There are two components to be aware of: the BoxMap component itself, and the OpqBoxLeafletMarkerManager component (I’m bad at naming things!) - which is a child component of the BoxMap. The purpose of this latter component is to encapsulate all the code related to the creation, management, and customization of box markers on the map.
I realized that there are 4 aspects of the map markers that future developers might be interested in customizing:
To make it as easy as possible to customize these, the OpqBoxLeafletMarkerManager component accepts a set of callback props for their creation. See the below image for details.
By using these callbacks, it should be pretty trivial to customize the contents of any of these labels. For example, this is the callback that we define (and pass to the markerClusterLabelFunc prop) to display the box count in each cluster (it’s quite simple!):
createClusterBoxCountLabel(clusterBoxIds) {
const boxCount = clusterBoxIds.length;
return `<div style='font-size: 26px;'><b>${boxCount}</b></div>`;
}By defining a set of callbacks in the BoxMap component, we now have a simple and versatile way of displaying any kind of data we wish in respect to any given OpqBox on the map. It would not be difficult, for example, to adopt the above code and create a separate callback for displaying the daily event count in each cluster.
When you run the BoxMap on branch issue-69, I’ve included code to demonstrate these four callbacks in action. They do not all have to be used, of course - I just wanted to demonstrate usage for your code review.
In the future, if we wanted to allow the the user to change the contents of ANY of these labels on demand, all you would have to do is pass the selected callback to the appropriate OpqBoxLeafletMarkerManager prop.
FYI: All of these label contents will update reactively, should they contain reactive content.
philipmjohnson
commented
6 years ago Bravo! Just took a quick skim through the code and it looks great! The only, totally minor thing I can see at first glance is the lack of jsdoc-style comments for each function and method. Eventually, I want to use something like esdoc to generate code-level documentation for view. When or if the fancy strikes you, you could do a documentation pass, but there's no hurry and the code is pretty easy to understand.
I am intrigued by a couple of things I saw, such as Lodash.flowRight, so you've piqued my interest in some language constructs I hadn't seen before. Well done!
philipmjohnson
commented
6 years ago Just noticed that the BoxMap component "clips" the Admin dropdown menu from the NavBar:
Note that I can still select the "hidden" items in the dropdown by clicking in the appropriate place in the boxmap...
aghalarp
commented
6 years ago I've pushed a fix to master; it was just a z-index conflict between the dropdown menu and the Leaflet map.
Please do work for this task in a branch called issue-69.
Upon logging into OPQView, users will have a link in their Navbar to a page called "Box Map". This page contains a single component called BoxMap. The component subscribes to the OPQBox collection, from which it obtains the latest location data for all of the boxes owned by this user.
The component then uses a React-based Mapping facility (react-leaflet?) to display the latest location of each box.
Some design issues:
Location representation: We want to be flexible regarding the way location data is specified. By default, we currently specify location with a zip code. However, for the Agile Power Monitoring Project, we will want to specify boxes with much greater precision so that the map can show the locations of boxes within buildings on campus. So, the Box Map component should be able to handle either zip code or lat/long coordinates. We will need to update the OPQBox data model to specify how lat/long data can be provided as an alternative representation for location.
Location lookup: It appears to be unnecessary to build in a lat-long finder. Users can be directed to any of a number of public websites (for example, https://www.latlong.net/) to obtain the lat/long coordinates for a particular location.
Box Data: It would be nice to provide a little bit of data about the box. Minimally, the marker associated with the box can provide as a pop-up the box's name, description, and the nickname associated with the last location. It could also specify its health status (up, down, unplugged). For more detail, the user should go to another component.