Issue 18#54
Conversation
…are specified, without spec_id filtering
…s returned by server
Creates a SpecDetails object using the constructor and ensures that it calls the correct methods to populate itself Running is simple ``` $ python -m unittest ```
Add simple unit test with a mocking example
shankari
left a comment
shankari
left a comment
There was a problem hiding this comment.
This looks good overall. I'd like to see a "Testing done" section indicating how you tested it. Would also be good to pull out the keys into constants, and to change the notebooks to use the constants.
| pv = PhoneView(sd) | ||
| for phone_os, phone_map in pv.map().items(): | ||
| for phone_label, phone_detail_map in phone_map.items(): | ||
| for key in [k for k in phone_detail_map.keys() if "/" in k]: |
There was a problem hiding this comment.
as part of your documentation, you should highlight that this is how you identify that a particular key needs to be saved to a file
| sd.eval_start_ts, | ||
| sd.eval_end_ts, | ||
| out_dir) | ||
| for ranges in [phone_detail_map["calibration_ranges"], phone_detail_map["evaluation_ranges"]]: |
There was a problem hiding this comment.
so we need two levels of dump_data_to_file to handle the transitions and then the data within each range? Again, add a comment to highlight that.
|
Next step is to change the notebooks to use |
shankari
left a comment
shankari
left a comment
There was a problem hiding this comment.
Looks pretty good overall. Just some minor fixes. I am open to merging with only this notebook, but you do need to change the other notebooks as well before starting on the tree flattening -> pandas shift
| " #for tr in r[\"evaluation_trip_ranges\"]:\n", | ||
| " # print(12 * ' ', 30 * \"-\")\n", | ||
| " # print(12 * ' ',tr[\"trip_id\"], tr.keys())\n", | ||
| " # for sr in tr[\"evaluation_section_ranges\"]:\n", | ||
| " # print(16 * ' ', 30 * \"~\")\n", | ||
| " # print(16 * ' ',sr[\"trip_id\"], sr.keys())" |
|
Notebooks are mostly working now except for the ones that rely on |
|
Confirmed that the generated maps don't change
|
| "About to retrieve messages using {'user': 'ucb-sdb-android-1', 'key_list': ['manual/evaluation_transition'], 'start_time': 1563606000, 'end_time': 1588204800}\n", | ||
| "response = <Response [200]>\n", | ||
| "Found 86 entries\n", |
There was a problem hiding this comment.
How are we still getting Response [200] here? Shouldn't we be using the FileSpecDetails?
| "execution_count": null, | ||
| "execution_count": 7, | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "outputs": [ | ||
| { | ||
| "name": "stdout", | ||
| "output_type": "stream", | ||
| "text": [ | ||
| "About to retrieve messages using {'user': 'shankari@eecs.berkeley.edu', 'key_list': ['config/evaluation_spec'], 'start_time': 0, 'end_time': 1613685967}\n", |
There was a problem hiding this comment.
Please don't check in outputs, at least for files that didn't have outputs to begin with.
There was a problem hiding this comment.
Don't change the XXX_master files that had outputs just yet. As you point out, we haven't figured out how to run the algorithms on in-memory data instead.
There was a problem hiding this comment.
Try and keep the PR as minimal as possible so it is easy to review and understand.
…lysis-scripts into issue18 merge fixes
|
@singhish Can you also revert the changes to the master ipynb and file a separate issue to track it? Then I can merge this change and we can discuss the complicated part in greater detail after you are done with the outline. |
|
Please make sure to handle the minor fixes at MobilityNet/mobilitynet.github.io#23 |
2 participants
See MobilityNet/mobilitynet.github.io#18.