Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vehicle type model REQUIRE_DATA_FOR_ALL_ALTS needs to be True #789

Closed
i-am-sijia opened this issue Feb 1, 2024 · 3 comments
Closed

Vehicle type model REQUIRE_DATA_FOR_ALL_ALTS needs to be True #789

i-am-sijia opened this issue Feb 1, 2024 · 3 comments
Labels
Bug Something isn't working/bug f

Comments

@i-am-sijia
Copy link
Contributor

Describe the bug

The vehicle type choice model creates alts_wide and alts_long tables, and they need to be in the same length and order, because this line of code assumes that for the final mapping of choices:

choices["vehicle_type"] = choices["vehicle_type"].map(dict(enumerate(alts)))

However, the section of code below drops alternatives from alts_wide if there are alternatives that do not exist in the input VEHICLE_TYPE_DATA_FILE and if the optional REQUIRE_DATA_FOR_ALL_ALTS does not get defined or is set to False.

if model_settings.get("REQUIRE_DATA_FOR_ALL_ALTS", False):
# fail if alternative does not have an associated record in the data
assert (
len(missing_alts) == 0
), f"missing vehicle data for alternatives:\n {missing_alts}"
else:
# eliminate alternatives if no vehicle type data
alts_wide = alts_wide[alts_wide._merge != "left_only"]

This will make the alts_wide inconsistent with the alts_long, and make the final mapping of choices incorrect.

The model will still run, but the result will be incorrect.

To Reproduce

Steps to reproduce the behavior:

  1. Delete the first row of the config/vehicle_type_data.csv
  2. Run the existing prototype_mtc_extended model
  3. The model will run, but it creates different result for vehicle type

Expected behavior

The model should fail if there are alternatives that do not exist in the vehicle type data. REQUIRE_DATA_FOR_ALL_ALTS should not be optional or and should be True. The IF condition on Line 217 can be replaced by the assert.

@i-am-sijia i-am-sijia added the Bug Something isn't working/bug f label Feb 1, 2024
@dhensle
Copy link
Contributor

dhensle commented Feb 2, 2024

Good find @i-am-sijia. I believe I have a fix (#790) in place to also filter alts_long if alts_wide gets filtered. I believe this means the REQUIRE_DATA_FOR_ALL_ALTS option is usable.

@i-am-sijia
Copy link
Contributor Author

Ok, let's add REQUIRE_DATA_FOR_ALL_ALTS to the model documentation, and update the following blurb to remove "must be included in the file" -

activitysim/docs/models.rst

Lines 448 to 451 in a8e755f

* ``VEHICLE_TYPE_DATA_FILE``: Filename for input vehicle type data. Must have columns ``body_type``, ``fuel_type``, and ``vehicle_year``.
Vehicle ``age`` is computed using the ``FLEET_YEAR`` option. Data for every alternative specified in the ``combinatorial_alts`` option must be included
in the file. Vehicle type data file will be joined to the alternatives and can be used in the utility expressions if ``PROBS_SPEC`` is not provided.
If ``PROBS_SPEC`` is provided, the vehicle type data will be joined after a vehicle type is decided so the data can be used in downstream models.

More importantly, users should be warned when the software is dropping vehicle type alternatives. In case someone adds vehicle types (e.g., AVs) in the yaml but forgets to add vehicle types in the vehicle type data.

@dhensle
Copy link
Contributor

dhensle commented Feb 9, 2024

Addressed with #790

@dhensle dhensle closed this as completed Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working/bug f
Projects
None yet
Development

No branches or pull requests

2 participants