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

Visualization Pull Request #532

Merged
merged 102 commits into from
Apr 12, 2022
Merged

Visualization Pull Request #532

merged 102 commits into from
Apr 12, 2022

Conversation

danielsclint
Copy link

@danielsclint danielsclint commented Dec 22, 2021

#443: Visualization
#533: np.MachAr Deprecation (Hotfix)

Currently in DRAFT

Todo

Review Criteria Responses

  1. Does it contain all the required elements, including a runnable example, documentation, and tests?
    Yes. A runnable example is incorporated into example_mtc. Documentation has been updated to explain yaml and csv expression files used to configure the summarize model. New tests were created to check csv outputs from a minimal example.

  2. Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
    Yes. The summarize model produces outputs in a flexible, industry-standard format (csv) and integrates fully into the Activitysim modeling pipeline. It does not impact travel modeling results, but operates on these results.

  3. Are the runtimes reasonable and does it provide documentation justifying this claim?
    Yes. The model adds only seconds to model runtime on a standard workstation.

  4. Does it include non-Python code, such as C/C++? If so, does it compile on any OS and are compilation instructions included?
    Yes and no. The changes to the ActivitySim package are completely implemented in Python, but it does add an additional dependency to SimWrapper.

  5. Is it licensed with the ActivitySim license that allows the code to be freely distributed and modified and includes attribution so that the ‘provenance’ of the code can be tracked? Does it include an official release of ownership from the funding agency if applicable?
    This work was done under contract to AMPO Research Foundation, and, presumably, AMPO Research Foundation is providing the changes without any additional licensing beyond the existing ActivitySim licensing.

  6. Does it appropriately interact with the data pipeline (i.e. it doesn't create new ways of managing data)?
    The model ingests data from the pipeline and produces csv files in a standard "output" directory.

  7. Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework?
    The model does not impact modeling outputs. As such, no regression testing has been done.

  8. Does it include sufficient test coverage and test data for existing and proposed features?
    Yes. It includes a new test module with associated configuration files and input data.

  9. Any other comments or suggestions for improving the developer experience?
    No

Clint Daniels and others added 30 commits December 21, 2021 16:51
…ed skim information that is lost right now.
@jpn-- jpn-- requested a review from JoeJimFlood March 17, 2022 16:06
@JoeJimFlood
Copy link
Contributor

One comment I will make: I feel as though the way the aggregation maps are currently defined isn't super user-friendly. For example, the major access trip mode in the MTC example it's currently defined as follows:

map:
        DRIVEALONEFREE: SOV
        DRIVEALONEPAY: SOV
        SHARED2FREE: HOV
        SHARED2PAY: HOV
        SHARED3FREE: HOV
        SHARED3PAY: HOV
        WALK_LOC: Walk to Transit
        WALK_LRF: Walk to Transit
        WALK_EXP: Walk to Transit
        WALK_HVY: Walk to Transit
        WALK_COM: Walk to Transit
        DRIVE_LOC: Drive to Transit
        DRIVE_LRF: Drive to Transit
        DRIVE_EXP: Drive to Transit
        DRIVE_HVY: Drive to Transit
        DRIVE_COM: Drive to Transit
        WALK: Non-Motorized
        BIKE: Non-Motorized
        TAXI: Ride Hail
        TNC_SINGLE: Ride Hail
        TNC_SHARED: Ride Hail

While this can be directly input into the pd.Series.map() function, the same information could be given by the following:

map:
    SOV:
        DRIVEALONEFREE
        DRIVEALONEPAY
    HOV:
        SHARED2FREE
        SHARED2PAY
        SHARED3FREE
        SHARED3PAY
    Walk to Transit:
        WALK_LOC
        WALK_LRF
        WALK_EXP
        WALK_HVY
        WALK_COM
    Drive to Transit:
        DRIVE_LOC
        DRIVE_LRF
        DRIVE_EXP
        DRIVE_HVY
        DRIVE_COM
    Non-Motorized:
        WALK
        BIKE
    Ride Hail:
        TAXI
        TNC_SINGLE
        TNC_SHARED

This is less repetitive and more readable. Something like this would then need to be added before line 289 of summarize.py and 290 would need to be changed to use map instead of agg['map'] (I can make the change if needed):

map = {}
for value in agg['map']:
    for key in agg['map'][value]:
        map[key] = value

@jpn-- jpn-- marked this pull request as ready for review April 7, 2022 14:59
@jpn-- jpn-- merged commit ef05447 into ActivitySim:develop Apr 12, 2022
@i-am-sijia i-am-sijia deleted the ft_vis_1 branch March 18, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet