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

Common settings for fastmath, sharrow_skip, and other compute controls across components #824

Merged
merged 25 commits into from
Apr 18, 2024

Conversation

jpn--
Copy link
Member

@jpn-- jpn-- commented Feb 29, 2024

The reason why the school escort model was failing with sharrow relates to the fastmath setting.

Part of how sharrow accelerates code evaluation is by turning on the "fastmath" compiler optimizations in Numba. Using this setting has numerous effects, but two are salient here:

  1. It makes the code run faster
  2. It can cause elementary operations to give unexpected results where one or more operands is NaN, e.g. normally evaluating x < 0 should result in False when x is NaN but with fastmath turned on the result might be True instead; the actual result of such an operation is not defined and might be hardware dependent.

We do not want to simply turn off fastmath, as the speed we get from (1) is desirable and the problems created by (2) don't actually occur in most models, as most data columns in our tables don't include missing values. But the school escort model DOES include missing values (e.g. the age of the 3rd child in a household with less than 3 children).

This PR introduces a sharrow_fastmath setting for all relevant components, with True as the default for nearly all Sharrow-compiled model components, but adds a mechanism to turn it off for individual components as needed. The school escort model has the default for this setting changed to False.

@jpn-- jpn-- marked this pull request as draft February 29, 2024 16:43
@jpn-- jpn-- self-assigned this Feb 29, 2024
@dhensle
Copy link
Contributor

dhensle commented Feb 29, 2024

Just wanted to make sure you knew that the string removal work was performed already here: ActivitySim/activitysim-prototype-mtc#4

@jpn-- jpn-- marked this pull request as ready for review March 27, 2024 15:30
Copy link
Contributor

@dhensle dhensle left a comment

Choose a reason for hiding this comment

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

There are a number of submodels which do not include this setting, e.g. destination models, scheduling, transit pass subsidy, etc. Those also need to be passed with the fastmath argument, especially since their yaml files will now accept the setting since the BaseLogitComponentSettings was updated.

activitysim/core/configuration/logit.py Outdated Show resolved Hide resolved
@jpn--
Copy link
Member Author

jpn-- commented Apr 1, 2024

@dhensle you make a good point. Also, while I'm pushing fastmath through all the components, I realized its convenient to wrap all component level sharrow controls together (i.e., also skip) to ensure a consistent user interface.

@jpn-- jpn-- requested a review from dhensle April 2, 2024 15:32
@jpn--
Copy link
Member Author

jpn-- commented Apr 15, 2024

@dhensle this PR is ready for your re-review. I believe I addressed your concerns, and then some.

@jpn-- jpn-- changed the title Make sharrow work for school escorting Common sharrow settings for fastmath and skip across components Apr 16, 2024
@jpn-- jpn-- changed the title Common sharrow settings for fastmath and skip across components Common settings for fastmath, sharrow_skip, and other compute controls across components Apr 17, 2024
@jpn--
Copy link
Member Author

jpn-- commented Apr 17, 2024

In thinking about the solution for #842, I realized we probably want these controls to be more general that "sharrow_settings", something more like "compute_settings". The reason we are seeing the overflow in #842 seems to be because we are using bottleneck for the computations, but if we activate compute.use_numexpr instead the problem goes away. If we make these controls more general, we can trigger this pandas setting here as well. This allows us to change compute settings in a consistent component-specific manner for non-sharrow compute. For now, that includes settings the pandas options compute.use_bottleneck, compute.use_numexpr, and compute.use_numba (see pandas.set_option).

@jpn--
Copy link
Member Author

jpn-- commented Apr 18, 2024

@dhensle Will you have time to finish a (re)review of this PR this morning so we can merge it? It is now blocking progress on #833 as we want to coordinate the settings.

Copy link
Contributor

@dhensle dhensle left a comment

Choose a reason for hiding this comment

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

I think you just missed the transit_pass_subsidy model. Other than that I think it looks good.

activitysim/core/interaction_simulate.py Outdated Show resolved Hide resolved
activitysim/core/interaction_simulate.py Outdated Show resolved Hide resolved
@jpn-- jpn-- merged commit 751ee44 into ActivitySim:main Apr 18, 2024
17 checks passed
Sign up for free to 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.

2 participants