Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings - #1094

Merged
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings
Aug 13, 2026
Merged

Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settings#1094
jpn-- merged 5 commits into
mainfrom
copilot/deprecate-chooser-columns-settings

Conversation

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS were memory hacks that manually pruned chooser tables before merging with alternatives. util.drop_unused_columns (on by default via compute_settings.drop_unused_columns) now does this automatically from the spec, so the manual lists are redundant — and a well-known gotcha, since omitting a column used by a spec silently breaks the model.

Settings

  • Both settings are now typed Any | None with a mode="before" field validator that emits a DeprecationWarning and discards the value. Existing configs continue to load rather than tripping extra="forbid".
  • Applies to TourLocationComponentSettings, TourModeComponentSettings, TourSchedulingSettings, and SchoolEscortSettings.

Component code

  • Removed the # FIXME - MEMORY HACK column subsetting (and the associated # Drop this when PR #1017 is mergedhousehold_id workarounds) from location_choice, util/tour_destination, util/tour_od, util/tour_scheduling, and school_escorting.
  • Deleted the now-unreachable helpers logsums.filter_chooser_columns and expressions.filter_chooser_columns.

Configs / docs / tests

  • Stripped both settings from the 46 bundled example config files.
  • New activitysim/core/test/test_deprecated_chooser_columns.py covering the warning per settings class and the no-warning-when-omitted case; updated the two existing tests that supplied the setting.
  • Change log entry under "Upcoming Changes".
>>>TourModeComponentSettings.model_validate(
... {"SPEC": "tour_mode_choice.csv", "LOGSUM_CHOOSER_COLUMNS": ["age"]}
... ).LOGSUM_CHOOSER_COLUMNSDeprecationWarning: LOGSUM_CHOOSER_COLUMNSisdeprecatedandnolongerused,
unusedcolumnsarenowdroppedautomaticallyNone

Users should remove these keys from their configs; no other action is required.

CopilotAI changed the title [WIP] Deprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsDeprecate SIMULATE_CHOOSER_COLUMNS and LOGSUM_CHOOSER_COLUMNS settingsJul 30, 2026
CopilotAI requested a review from jpn--July 30, 2026 21:18
@jpn--

Copy link
Copy Markdown
Member

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

@jpn--

Copy link
Copy Markdown
Member

@copilot, address review comments on this PR

CopilotAI commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During househ...

Fixed in 71f88ba by taking the second option: _interaction_sample no longer bypasses pruning when trace targets exist. It now always runs util.drop_unused_columns on both choosers and alternatives when compute_settings.drop_unused_columns is enabled, adding the trace slicer id columns (person_id, proto_person_id, proto_household_id; household_id is already always retained by drop_unused_columns) to the protected list so interaction_trace_rows can still identify traced rows.

The full, unpruned choosers and alternatives tables are still written to the trace output earlier in the function, so nothing is lost from the trace except extra unused columns in the interaction_df dump. Estimation paths are untouched — interaction_sample passes estimator=None to utility evaluation and the other call sites that guard on estimator is None were not changed.

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

• activitysim/abm/models/util/tour_destination.py:697 — The change merges all persons_merged columns into tours instead of retaining the previous narrow chooser-column selection. During household tracing, interaction_sample disables automatic chooser-column pruning, then cross-joins these wide choosers with alternatives. Because persons_merged can include household, land-use, accessibility, and disaggregate-accessibility fields, destination sampling can regress to a large chooser×alternative memory footprint and potentially OOM realistic traced runs.

Retain a safe explicit chooser-column subset for trace/estimation paths, or change pruning to preserve only trace-required fields rather than bypassing pruning completely.

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

This setting is now obsolete and does nothing. Its functionality has been
replaced by :func:`activitysim.core.util.drop_unused_columns`.

.. deprecated:: 1.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current release is v1.5, so this will be officially deprecated as of the next release (1.6)

@jpn--jpn-- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. The SANDAG test is failing due to the bug in at-work tour logsums.

choosers[ORIG_TAZ] = network_los.map_maz_to_taz(choosers[orig_maz])
# This is the TAZ for the configured tour origin. A wider chooser table
# may already contain a same-named home TAZ, which is incorrect for models
# such as at-work subtour destination choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like the AI (here, OpenAI's Sol) found and fixed a mostly unrelated bug.

@jpn--
jpn-- requested review from dhensle and janzillAugust 4, 2026 02:03
@jpn--

jpn-- commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dhensle, @janzill I don't think a comprehensive review from both of you here is totally necessary, but I know each of you has been in some of these files recently (and this is a re-do of David's original PR from some time ago) so I want to give you an opportunity to comment / review before I finalize and merge this.

@jpn--
jpn-- marked this pull request as ready for review August 4, 2026 02:06
@jpn--jpn-- moved this to Under Review in Phase 11Aug 4, 2026

@janzilljanzill 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.

this all looks good, no suggestions but one comment: Sol did well finding that at-work origin bug.

@dhensledhensle 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.

This looks good to me too. Glad to see copilot kept the traceable id columns which I think has previously been an issue that was also fixed as part of this PR. This is not tested as part of this, but we should really add some better tracing test coverage. I think that is better done in a separate PR.

@jpn--
jpn-- merged commit 131f09a into mainAug 13, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from Under Review to Done in Phase 11Aug 13, 2026
@jpn--
jpn-- deleted the copilot/deprecate-chooser-columns-settings branch August 13, 2026 19:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Simulate Chooser Columns and Logsum Chooser Columns Settings Not needed

4 participants

@jpn--@janzill@dhensle