Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SFPE Handbook Chapter 59 to the Rust sfpe_handbook crate and exposes the implemented equations (59.5–59.9) through the PyO3 ofire Python API, with Sphinx API documentation updates.
Changes:
- Introduces Chapter 59 Rust modules with equation implementations and unit tests for equations 59.5–59.9.
- Adds Python bindings (PyO3) for Chapter 59 equations and wires the new chapter into the
sfpe_handbookPython module. - Updates Sphinx docs to include Chapter 59 modules; includes minor formatting cleanups in Chapter 50 files.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/sfpe_handbook/src/lib.rs | Exposes chapter_59 from the Rust SFPE Handbook crate. |
| crates/sfpe_handbook/src/chapter_59.rs | Declares Chapter 59 equation submodules. |
| crates/sfpe_handbook/src/chapter_59/equation_59_5.rs | Implements Eq. 59.5 (travel speed) + tests + equation-string helper. |
| crates/sfpe_handbook/src/chapter_59/equation_59_6.rs | Implements Eq. 59.6 (specific flow) + tests + equation-string helper. |
| crates/sfpe_handbook/src/chapter_59/equation_59_7.rs | Implements Eq. 59.7 (combined specific flow) + tests + equation-string helper. |
| crates/sfpe_handbook/src/chapter_59/equation_59_8.rs | Implements Eq. 59.8 (calculated flow) + tests + equation-string helper. |
| crates/sfpe_handbook/src/chapter_59/equation_59_9.rs | Implements Eq. 59.9 (combined calculated flow) + tests + equation-string helper. |
| crates/sfpe_handbook/src/chapter_59/equation_59_10.rs | Adds an unimplemented placeholder (currently panics). |
| crates/sfpe_handbook/src/chapter_59/equation_59_11.rs | Adds an unimplemented placeholder (currently panics). |
| crates/sfpe_handbook/src/chapter_50/equation_50_17.rs | Formatting-only change to the equation-string helper signature. |
| crates/sfpe_handbook/src/chapter_50/equation_50_16.rs | Formatting-only whitespace cleanup. |
| crates/python_api/src/sfpe_handbook.rs | Exposes chapter_59 in the Python API sfpe_handbook module. |
| crates/python_api/src/sfpe_handbook/chapter_59.rs | Adds Chapter 59 Python submodule wiring (wrap_pymodule). |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_5.rs | PyO3 binding + docstring for Eq. 59.5. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_6.rs | PyO3 binding + docstring for Eq. 59.6. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_7.rs | PyO3 binding + docstring for Eq. 59.7. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_8.rs | PyO3 binding + docstring for Eq. 59.8. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_9.rs | PyO3 binding + docstring for Eq. 59.9. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_10.rs | PyO3 placeholder returning NotImplementedError + placeholder docs. |
| crates/python_api/src/sfpe_handbook/chapter_59/equation_59_11.rs | PyO3 placeholder returning NotImplementedError + placeholder docs. |
| crates/python_api/src/sfpe_handbook/chapter_50/equation_50_20.rs | Formatting-only trailing brace alignment. |
| crates/python_api/src/sfpe_handbook/chapter_50/equation_50_17.rs | Formatting-only trailing brace alignment. |
| crates/python_api/docs/api/sfpe-handbook.rst | Adds Chapter 59 API documentation entries. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+17
to
+19
| #[test] | ||
| #[ignore = "placeholder - implement when equation is added"] | ||
| fn test_equation_59_11() { |
There was a problem hiding this comment.
The test in this module is #[ignore], leaving the newly added placeholder function uncovered. Prefer not landing placeholder calculation functions in the crate until they can be implemented and tested (or add non-ignored tests once implemented) to keep calculation coverage at 100%.
Comment on lines
+4
to
+25
| /// Placeholder for equation 59.10. | ||
| /// | ||
| /// This function is a placeholder and will be implemented with the actual | ||
| /// equation logic. | ||
| /// | ||
| /// .. math:: | ||
| /// | ||
| /// \text{TODO: Add equation} | ||
| /// | ||
| /// where: | ||
| /// | ||
| /// - TODO: Add variable definitions | ||
| /// | ||
| /// Args: | ||
| /// TODO: Add arguments | ||
| /// | ||
| /// Returns: | ||
| /// float: TODO: Add return description | ||
| /// | ||
| /// Example: | ||
| /// >>> import ofire | ||
| /// >>> # TODO: Add example when equation is implemented |
There was a problem hiding this comment.
This module is exposed via autodoc but the docstring is mostly TODO placeholders (equation/variables/args/returns). Consider removing Equation 59.10 from the Python module/docs until implemented, or fully documenting the intended signature and variables now so published docs aren’t incomplete.
Suggested change
| /// Placeholder for equation 59.10. | |
| /// | |
| /// This function is a placeholder and will be implemented with the actual | |
| /// equation logic. | |
| /// | |
| /// .. math:: | |
| /// | |
| /// \text{TODO: Add equation} | |
| /// | |
| /// where: | |
| /// | |
| /// - TODO: Add variable definitions | |
| /// | |
| /// Args: | |
| /// TODO: Add arguments | |
| /// | |
| /// Returns: | |
| /// float: TODO: Add return description | |
| /// | |
| /// Example: | |
| /// >>> import ofire | |
| /// >>> # TODO: Add example when equation is implemented | |
| /// SFPE Handbook Equation 59.10 (placeholder). | |
| /// | |
| /// This function is a placeholder for Equation 59.10 from the SFPE Handbook. | |
| /// The equation has not yet been implemented, and this function will always | |
| /// raise :class:`NotImplementedError` when called. | |
| /// | |
| /// Example: | |
| /// >>> import ofire | |
| /// >>> from ofire.sfpe_handbook.chapter_59 import equation_59_10 | |
| /// >>> try: | |
| /// ... equation_59_10.equation_59_10_placeholder() | |
| /// ... except NotImplementedError: | |
| /// ... print("Equation 59.10 is not yet implemented") |
Comment on lines
+4
to
+25
| /// Placeholder for equation 59.11. | ||
| /// | ||
| /// This function is a placeholder and will be implemented with the actual | ||
| /// equation logic. | ||
| /// | ||
| /// .. math:: | ||
| /// | ||
| /// \text{TODO: Add equation} | ||
| /// | ||
| /// where: | ||
| /// | ||
| /// - TODO: Add variable definitions | ||
| /// | ||
| /// Args: | ||
| /// TODO: Add arguments | ||
| /// | ||
| /// Returns: | ||
| /// float: TODO: Add return description | ||
| /// | ||
| /// Example: | ||
| /// >>> import ofire | ||
| /// >>> # TODO: Add example when equation is implemented |
There was a problem hiding this comment.
This module is exposed via autodoc but the docstring is mostly TODO placeholders (equation/variables/args/returns). Consider removing Equation 59.11 from the Python module/docs until implemented, or fully documenting the intended signature and variables now so published docs aren’t incomplete.
Suggested change
| /// Placeholder for equation 59.11. | |
| /// | |
| /// This function is a placeholder and will be implemented with the actual | |
| /// equation logic. | |
| /// | |
| /// .. math:: | |
| /// | |
| /// \text{TODO: Add equation} | |
| /// | |
| /// where: | |
| /// | |
| /// - TODO: Add variable definitions | |
| /// | |
| /// Args: | |
| /// TODO: Add arguments | |
| /// | |
| /// Returns: | |
| /// float: TODO: Add return description | |
| /// | |
| /// Example: | |
| /// >>> import ofire | |
| /// >>> # TODO: Add example when equation is implemented | |
| /// Equation 59.11 placeholder – not yet implemented. | |
| /// | |
| /// This Python binding reserves the API name for Equation 59.11 from the | |
| /// SFPE Handbook, Chapter 59. The underlying engineering calculation has | |
| /// not yet been implemented in this library. | |
| /// | |
| /// Calling this function will always raise a :class:`NotImplementedError` | |
| /// in Python to indicate that the equation is currently unavailable. | |
| /// | |
| /// Example: | |
| /// >>> import ofire | |
| /// >>> from ofire.sfpe_handbook import chapter_59 | |
| /// >>> # This call will raise NotImplementedError until implemented | |
| /// >>> chapter_59.equation_59_11.equation_59_11_placeholder() |
Comment on lines
+13
to
+15
| /// Chapter 59 - Placeholder equations. | ||
| /// | ||
| /// This chapter contains placeholder equations that will be implemented. |
There was a problem hiding this comment.
The module-level docstring says this chapter contains “placeholder equations”, but equations 59.5–59.9 are implemented and documented. Consider updating the chapter description to reflect the hydraulic/egress flow model and explicitly note which equations are still unimplemented (59.10/59.11).
Suggested change
| /// Chapter 59 - Placeholder equations. | |
| /// | |
| /// This chapter contains placeholder equations that will be implemented. | |
| /// Chapter 59 - Hydraulic egress flow model. | |
| /// | |
| /// This chapter exposes equations for the hydraulic model of occupant flow | |
| /// (Equations 59.5–59.9). Equations 59.10 and 59.11 are currently provided | |
| /// as placeholders and may not be fully implemented. |
Comment on lines
+14
to
+25
| /// - :math:`S` is the speed along the line of travel | ||
| /// - :math:`k` is the speed constant from Table 59.2 | ||
| /// - :math:`a` is the unit conversion constant (0.266 for SI, 2.86 for imperial) | ||
| /// - :math:`D` is the population density in persons per unit area | ||
| /// | ||
| /// Args: | ||
| /// k (float): Speed constant from Table 59.2 | ||
| /// a (float): Unit conversion constant (0.266 for SI, 2.86 for imperial) | ||
| /// d (float): Population density (persons per unit area) | ||
| /// | ||
| /// Returns: | ||
| /// float: Speed S along the line of travel |
There was a problem hiding this comment.
In the where: section, the variable definitions omit units/dimensionless markers (unlike other SFPE docstrings in this repo). Adding units for S, k, a, and D would make the equation easier to apply correctly across SI vs imperial usage.
Suggested change
| /// - :math:`S` is the speed along the line of travel | |
| /// - :math:`k` is the speed constant from Table 59.2 | |
| /// - :math:`a` is the unit conversion constant (0.266 for SI, 2.86 for imperial) | |
| /// - :math:`D` is the population density in persons per unit area | |
| /// | |
| /// Args: | |
| /// k (float): Speed constant from Table 59.2 | |
| /// a (float): Unit conversion constant (0.266 for SI, 2.86 for imperial) | |
| /// d (float): Population density (persons per unit area) | |
| /// | |
| /// Returns: | |
| /// float: Speed S along the line of travel | |
| /// - :math:`S` is the speed along the line of travel (m/s or ft/s, consistent with :math:`k`) | |
| /// - :math:`k` is the speed constant from Table 59.2 (m/s or ft/s) | |
| /// - :math:`a` is the unit conversion constant (m²/person for SI, ft²/person for imperial) | |
| /// - :math:`D` is the population density (persons/m² or persons/ft²) | |
| /// | |
| /// Args: | |
| /// k (float): Speed constant from Table 59.2 (m/s or ft/s) | |
| /// a (float): Unit conversion constant (m²/person for SI, ft²/person for imperial) | |
| /// d (float): Population density (persons/m² or persons/ft²) | |
| /// | |
| /// Returns: | |
| /// float: Speed :math:`S` along the line of travel (m/s or ft/s, consistent with :math:`k`) |
Comment on lines
+18
to
+25
| /// - :math:`a` is the unit conversion constant (0.266 for SI, 2.86 for imperial) | ||
| /// - :math:`D` is the population density (persons per unit area) | ||
| /// - :math:`k` is the speed constant from Table 59.2 | ||
| /// | ||
| /// Args: | ||
| /// a (float): Unit conversion constant (0.266 for SI units, 2.86 for imperial units) | ||
| /// d (float): Population density (persons/m² or persons/ft²) | ||
| /// k (float): Speed constant from Table 59.2 |
There was a problem hiding this comment.
The where: section omits units/dimensionless markers for a, D, and k. Adding units (or explicitly stating dimensionless where applicable) would make the doc consistent with other SFPE equation docstrings and reduce ambiguity between SI vs imperial inputs.
Suggested change
| /// - :math:`a` is the unit conversion constant (0.266 for SI, 2.86 for imperial) | |
| /// - :math:`D` is the population density (persons per unit area) | |
| /// - :math:`k` is the speed constant from Table 59.2 | |
| /// | |
| /// Args: | |
| /// a (float): Unit conversion constant (0.266 for SI units, 2.86 for imperial units) | |
| /// d (float): Population density (persons/m² or persons/ft²) | |
| /// k (float): Speed constant from Table 59.2 | |
| /// - :math:`a` is the unit conversion constant (m²/person or ft²/person; 0.266 for SI, 2.86 for imperial) | |
| /// - :math:`D` is the population density (persons/m² or persons/ft²) | |
| /// - :math:`k` is the speed constant from Table 59.2 (m/s or ft/min, consistent with the chosen unit system) | |
| /// | |
| /// Args: | |
| /// a (float): Unit conversion constant (m²/person or ft²/person; 0.266 for SI units, 2.86 for imperial units) | |
| /// d (float): Population density (persons/m² or persons/ft²) | |
| /// k (float): Speed constant from Table 59.2 (m/s or ft/min, consistent with the chosen unit system) |
Comment on lines
+4
to
+6
| pub fn equation_59_11_placeholder() -> f64 { | ||
| todo!("Implement equation 59.11") | ||
| } |
There was a problem hiding this comment.
equation_59_11_placeholder() is part of the public API but always panics via todo!(). Consider not exporting this equation/module until it’s implemented, or returning a non-panicking error type so callers don’t get a runtime panic.
Comment on lines
+150
to
+199
| Equation 59.5 | ||
| ~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_5 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.6 | ||
| ~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_6 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.7 | ||
| ~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_7 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.8 | ||
| ~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_8 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.9 | ||
| ~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_9 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.10 | ||
| ~~~~~~~~~~~~~~ | ||
|
|
||
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_10 | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
||
| Equation 59.11 | ||
| ~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
The new Chapter 59 section uses headings like “Equation 59.5” without short descriptions, while the rest of this file uses the “Equation X.Y - Description” heading style. Consider adding brief descriptions to these Chapter 59 equation headings (and adjust underline lengths accordingly) to keep the API docs consistent and scannable.
Suggested change
| Equation 59.5 | |
| ~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_5 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.6 | |
| ~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_6 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.7 | |
| ~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_7 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.8 | |
| ~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_8 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.9 | |
| ~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_9 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.10 | |
| ~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_10 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.11 | |
| ~~~~~~~~~~~~~~ | |
| Equation 59.5 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_5 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.6 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_6 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.7 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_7 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.8 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_8 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.9 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_9 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.10 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. automodule:: ofire.sfpe_handbook.chapter_59.equation_59_10 | |
| :members: | |
| :undoc-members: | |
| :show-inheritance: | |
| Equation 59.11 - SFPE Handbook Chapter 59 correlation | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
Comment on lines
+3
to
+7
| /// TODO: Implement the actual equation logic. | ||
| pub fn equation_59_10_placeholder() -> f64 { | ||
| todo!("Implement equation 59.10") | ||
| } | ||
|
|
There was a problem hiding this comment.
equation_59_10_placeholder() is part of the public API but always panics via todo!(). If this equation isn’t implemented yet, it’s safer to not export it (and its module) until it’s ready, or change the API to return a non-panicking error (e.g., Result) so downstream users can handle it gracefully.
Suggested change
| /// TODO: Implement the actual equation logic. | |
| pub fn equation_59_10_placeholder() -> f64 { | |
| todo!("Implement equation 59.10") | |
| } | |
| /// This function is currently not implemented and returns an error. | |
| /// When equation 59.10 is implemented, this function should perform | |
| /// the calculation and return the resulting value. | |
| pub fn equation_59_10_placeholder() -> Result<f64, Equation59_10Error> { | |
| Err(Equation59_10Error::NotImplemented) | |
| } | |
| /// Error type for the placeholder implementation of equation 59.10. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| pub enum Equation59_10Error { | |
| /// Indicates that equation 59.10 has not yet been implemented. | |
| NotImplemented, | |
| } |
Comment on lines
+17
to
+19
| #[test] | ||
| #[ignore = "placeholder - implement when equation is added"] | ||
| fn test_equation_59_10() { |
There was a problem hiding this comment.
The test in this module is #[ignore], leaving the newly added placeholder function uncovered. Prefer not landing placeholder calculation functions in the crate until they can be implemented and tested (or add non-ignored tests once implemented) to keep calculation coverage at 100%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Equations for 1st order hydraulic model. Still missing 59-10 and 59-11