provide more flexibility for time periods.

[danielsclint]

A Pull Request to address: #274. This requires changing some of the documentation and settings.yaml labels.

skim_time_periods:
    # 1440 minutes in a 24 hour period
    time_window: 1440 
    # 30 minute time-of-day choice windows
    period_minutes: 30 
    periods:
        - 0 # 12am
        - 12 # 6am
        - 22 # 11am
        - 32 # 4pm
        - 40 # 8pm
        - 48 # 12am
    labels:
        - EA
        - AM
        - MD
        - PM
        - EV

Review Criteria Responses

1. Does it contain all the required elements, including a runnable example, documentation, and tests?
   The examples are updated with the new setting.yaml configuration keywords. The documentation has also been updated to add a more robust description of the configuration keywords. No new tests were created, but the existing tests all pass.

2. Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
   Yes, it provides more flexibility to support any time period and duration the user or entity desires. This extends the spirit of the ActivitySim project to provide maximum implementation flexibility while maintaining a common workflow and model specification.

3. Are the runtimes reasonable and does it provide documentation justifying this claim?
   This change has no material impact on runtimes.

4. Does it include non-Python code, such as C/C++? If so, does it compile on any OS and are compilation instructions included?
   No. This is a Python-only change.

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 ARC, and, presumably, ARC 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)?
   This change does not impact the data pipeline.

7. Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework?
   No regression testing has been done. The code change removes a hard-coded value in the Python code with a set of user-defined variables. If the user specifies the same values as the previously hard-coded values, they should get the same results. The unit tests seem to confirm this assertion.

8. Does it include sufficient test coverage and test data for existing and proposed features?
   The test configuration files were modified to use the newest features.

9. Any other comments or suggestions for improving the developer experience?
   No. This is pretty straightforward.

[coveralls]

Coverage increased (+0.08%) to 86.21% when pulling d3597cd on danielsclint:ft_time_period_flex into 7db24a6 on ActivitySim:develop.

[danielsclint]

I revised the Pull Request to make it backwards compatible with existing implementations. I added default values in the code, and I left the old hours label in place with a FutureWarning advising folks to replace it in future updates of their code.

[bstabler]

Thanks @danielsclint, this is looking good. Our Contribution Review process includes a list of questions that we'd like you to answer in the PR thread. Once answered, then we'll provide feedback.
This is our first significant PR so thanks in advance for helping us pilot a process.

[danielsclint]

I edited the initial pull request comment to include some additional documentation and respond to the Contribution Review.

[bstabler]

@danielsclint - this looks good but we have one question - did you test it for a non 60 minute period setup? If so, then we'll accept this on your word since it is a small change.

We should require additional test coverage to maintain this functionality in the future. Another way of saying this is by adding additional tests we can guarantee we won't break activitysim in the future for the ARC (non 60 minute) setup. Do you have a small subset of the ARC model setup that you can start contributing to activitysim as you build out the model and add functionality? If so, please create and contribute an example_arc (?) folder.

I also think it is a good idea to setup a conversation between the ARC team and the ActivitySim team to discuss this idea in more detail. Maybe you can do this by coordinating with @guyrousseau? Thanks.

[joecastiglione]

Clint and Ben, Thank you (respectively) for both these contributions and the review. Ben - following up on a discussion from when we were scoping - I would like to better understand how test coverage is currently structured. Could we add this as an agenda time for our next partners' meeting, or alternatively could we set up a separate meeting with interested parties, if more time is needed? Thanks again, Joe

…

On Tue, Dec 17, 2019 at 3:13 PM Ben Stabler ***@***.***> wrote: @danielsclint <https://github.com/danielsclint> - this looks good but we have one question - did you test it for a non 60 minute period setup? If so, then we'll accept this on your word since it is a small change. We should require additional test coverage to maintain this functionality in the future. Another way of saying this is by adding additional tests we can guarantee we won't break activitysim in the future for the ARC (non 60 minute) setup. Do you have a small (subset) of the ARC model setup that you can start contributing to activitysim as you build out the model and add functionality? If so, please create and contribute an example_arc (?) folder. I also think it is a good idea to setup a conversation between the ARC team and the ActivitySim team to discuss this idea in more detail. Maybe you can do this by coordinating with @guyrousseau <https://github.com/guyrousseau>? Thanks. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#276?email_source=notifications&email_token=ABDVIH25XCBETSLQRTWR47DQZFMJ5A5CNFSM4JWA4NRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEI3VA#issuecomment-566791636>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDVIH4UXKPMIKATIXXDIO3QZFMJ5ANCNFSM4JWA4NRA> .

[bstabler]

@joecastiglione - sure, I'll add it to the partner call agenda and we can have a more in-depth follow-on discussion if needed.

[danielsclint]

@bstabler: Let me make a couple of additional changes to improve the robustness of the tests. As a prototype collaborative PR, I want to set a good example and a high standard for those that follow. I'll get an update to you in the next 1-2 days.

@joecastiglione: Let me know if you would like for me to participate in that discussion to talk about these PRs directly or the more general (and positive) experience of contributing to the code base.

[joecastiglione]

@danielsclint: Thanks, yes, I think it would really valuable for you to participate in this discussion(s).

…

On Wed, Dec 18, 2019 at 11:32 AM Clint Daniels ***@***.***> wrote: @bstabler <https://github.com/bstabler>: Let me make a couple of additional changes to improve the robustness of the tests. As a prototype collaborative PR, I want to set a good example and a high standard for those that follow. I'll get an update to you in the next 1-2 days. @joecastiglione <https://github.com/joecastiglione>: Let me know if you would like for me to participate in that discussion to talk about these PRs directly or the more general (and positive) experience of contributing to the code base. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#276?email_source=notifications&email_token=ABDVIHZIQG65BZZ3BW75PFLQZJ3GLA5CNFSM4JWA4NRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHHHEIQ#issuecomment-567177762>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDVIH6FB45BJC5JPQQDGVLQZJ3GLANCNFSM4JWA4NRA> .

[guyrousseau]

@danielsclint : Yes indeed Clint, it would be great if you could join our next ActivitySim call

[danielsclint]

This code now has extensive tests. The tests did result in one change (or at least re-implementing a change I thought had made). A good reminder that tests are always good.