[janzill]

Addresses #1129

[jpn--]

@janzill will update this PR to accommodate changes to PnR

[janzill]

@janzill will update this PR to accommodate changes to PnR

@jpn-- I've pushed my changes, it is ready for review. The PR touches many files, but quite a few of these are small consistency edits, most of which where caught by an LLM. This includes things like guarding against all choosers having no alternatives to sample from, which realistically only happens during testing and maybe for small-scale example models, addition of doc strings, and other small edge cases.

[jpn--]

Requesting changes for one problem in the skip-failed sampling path. Reviewed and reproduced against 2d6043a0e4b7e99300bb5adebecd329abda00fbf.

My previous comment here also identified another problem with tracing and skipped households, but I looked at it more closely and determined it is a new different bug surfaced by AI during my review, which should be resolved seperately.

Guard absent sample tables when every destination chooser fails

When all choosers fail sampling, run_tour_destination returns an empty choices frame and save_sample_df=None. With destination sample-table output enabled, activitysim/abm/models/joint_tour_destination.py:112-113 unconditionally accesses save_sample_df.index. non_mandatory_destination.py:137-138 has the same pattern.

Please guard the absent sample result in the callers and test all-failed sampling with sample-table output enabled. An unaffected tour from another household can remain, so the workflow's separate all-rows-dropped safeguard does not prevent this case.

This reproducer runs the real joint-tour step and stubs only the destination runner to return its all-failed result and record the skipped household. Save as repro_pr1130_empty_sample_table.py and run with this PR revision importable, e.g. PYTHONPATH=/path/to/pr-checkout python repro_pr1130_empty_sample_table.py.

Reproducer 2: absent sample-table crash

"""Reproduce the empty sample-table crash on PR #1130 (2d6043a).

Run with that ActivitySim revision on PYTHONPATH. No model data is needed.
Only the destination runner is stubbed, returning its all-failed result.
The real joint-tour step crashes when it tries to save the missing sample.
"""

from pathlib import Path
from tempfile import TemporaryDirectory
from unittest.mock import patch

import pandas as pd

from activitysim.abm.models import joint_tour_destination
from activitysim.core import logit, workflow
from activitysim.core.configuration.logit import TourLocationComponentSettings


def all_sampling_failed(state, tours, *args, **kwargs):
    """Match run_tour_destination's bookkeeping and return value when all fail."""
    logit.record_skipped_households(state, tours.household_id, "sample")
    return pd.DataFrame(columns=["choice", "logsum"]), None


def reproduce():
    """Run the real caller with saved samples enabled and one failed joint tour."""
    with TemporaryDirectory() as directory:
        root = Path(directory)
        (root / "configs").mkdir()
        (root / "data").mkdir()
        state = workflow.State().default_settings()
        state.initialize_filesystem(working_dir=root, configs_dir=("configs",))
        state.settings.skip_failed_choices = True
        state.settings.want_dest_choice_sample_tables = True

        # Keep an unaffected household's non-joint tour so dropping the failed
        # joint tour does not trigger the separate "all rows dropped" safeguard.
        tours = pd.DataFrame(
            {
                "household_id": [11, 12],
                "person_id": [1, 2],
                "tour_category": ["joint", "non_mandatory"],
                "destination": [-1, 20],
            },
            index=pd.Index([101, 102], name="tour_id"),
        )
        state.add_table("tours", tours)
        settings = TourLocationComponentSettings.model_construct(
            DEST_CHOICE_LOGSUM_COLUMN_NAME=None,
            DEST_CHOICE_SAMPLE_TABLE_NAME="joint_destination_sample",
            ALT_DEST_COL_NAME="zone_id",
        )

        with patch.object(
            joint_tour_destination.tour_destination,
            "run_tour_destination",
            all_sampling_failed,
        ):
            joint_tour_destination.joint_tour_destination(
                state,
                tours=state.get_dataframe("tours"),
                persons_merged=pd.DataFrame(),
                network_los=None,
                model_settings=settings,
            )

        # Expected after a fix: the unaffected tour survives without a crash.
        assert state.get_dataframe("tours").index.tolist() == [102]


if __name__ == "__main__":
    reproduce()

Observed at joint_tour_destination.py:113:

AttributeError: 'NoneType' object has no attribute 'index'

Testing and documentation

The added coverage is substantial. A local focused run across the nine affected test modules completed with 187 passed and 9 xfailed, and all executed GitHub CI checks were successful when reviewed (the documentation build was skipped). Both reproducer files also passed the repository's pre-commit hooks.

The expected failures explicitly leave the float32 underflow/stabilization portion of #1129 unresolved: valid low utilities can still result in skipped households. This is an existing limitation, not a new regression introduced here. Please clarify that narrower scope in the PR description, which currently only says “Addresses #1129”, and include a release note describing the skip-path fixes and the new requirement to supply zero_prob_choice_val when allow_zero_probs=True (except logsum-only skip_choice=True calls to interaction_sample_simulate).