Skip to content

post EET merge clean up - #1098

Open
janzill wants to merge 5 commits into
ActivitySim:mainfrom
outerl:jzill/eet_post_merge_clean_up
Open

post EET merge clean up#1098
janzill wants to merge 5 commits into
ActivitySim:mainfrom
outerl:jzill/eet_post_merge_clean_up

Conversation

@janzill

@janzill janzill commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Several small fixes and clean ups:

  1. Documentation didn't render math equations because mathjax was not enabled
  2. After our sampling discussions I realized that there is an edge case when Poisson sampling comes up empty and the number of available alternatives is smaller than the sample_size. Added a fix and a unit test.
  3. Now that the branch is public-facing we should use standard simulation terminology instead of our informal use of monte carlo for sampling from the inverse cdf with a standard uniform, see https://en.wikipedia.org/wiki/Inverse_transform_sampling. This mainly concerns the naming of sampling methods, and I chose "inverse_cdf".

@jpn-- regarding renaming a settings parameter I assumed that it's ok without deprecation in this case because the parameter was introduced less than 24h ago and there hasn't been a release yet, but let me know if you think otherwise.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates ActivitySim’s documentation and code to (1) render math in the docs, (2) harden Poisson sampling’s empty-draw fallback behavior, and (3) standardize terminology by renaming the probability-based sampling method to inverse_cdf.

Changes:

  • Enable MyST dollar-math support for Sphinx docs and update docs text accordingly.
  • Fix Poisson sampling fallback to exclude unavailable (zero-probability) alternatives; add a focused unit test.
  • Rename monte_carlo sampling method to inverse_cdf across docs, configuration typing, and tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
docs/users-guide/ways_to_run.rst Updates simulation-method terminology in the user guide.
docs/users-guide/sampling-methods.rst Renames documented sampling method default/option to inverse_cdf.
docs/dev-guide/sampling-methods.md Updates dev docs to match inverse_cdf naming and clarifies Poisson fallback behavior.
docs/dev-guide/explicit-error-terms.md Updates EET documentation terminology and math formatting.
docs/conf.py Enables MyST dollarmath extension for $...$ / $$...$$ rendering.
activitysim/core/test/test_logit.py Updates test comments/terminology to inverse-CDF.
activitysim/core/test/test_interaction_sample.py Adds/updates tests for Poisson fallback behavior and renamed method key.
activitysim/core/logit.py Adds a guard against unsupported nested-logit sampling inputs.
activitysim/core/interaction_simulate.py Removes outdated “Monte Carlo fashion” phrasing from docstrings.
activitysim/core/interaction_sample.py Renames sampling method literal/defaults; fixes Poisson fallback padding with unavailable alts.
activitysim/core/interaction_sample_simulate.py Removes outdated “Monte Carlo fashion” phrasing and expands EET-alignment commentary.
activitysim/core/configuration/top.py Updates configuration typing/docs for sample_method rename.
activitysim/core/configuration/base.py Updates compute-settings typing/docs for sample_method rename.
activitysim/abm/models/util/bias_logsums.py Updates warning text to refer to inverse_cdf instead of monte_carlo.
activitysim/abm/models/trip_scheduling_choice.py Clarifies EET alignment behavior and limitations in trip scheduling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread activitysim/core/logit.py Outdated
Comment thread docs/dev-guide/explicit-error-terms.md Outdated
@jpn--

jpn-- commented Aug 5, 2026

Copy link
Copy Markdown
Member

@janzill I agree no deprecation warnings are needed here. The two copilot review comments are both minor but I think reasonable. I tried to simply accept them so as to merge but I think I lack write access to the outerl fork. If you can accept them I will merge.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@janzill

janzill commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Agreed, they are reasonable so I accepted both, thanks Jeff

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.

3 participants