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

Added discrete states (Closes #117) #178

Merged
merged 15 commits into from
Apr 2, 2025
Merged

Conversation

teubert
Copy link
Contributor

@teubert teubert commented Dec 11, 2024

Closes #177

Adds the option of discrete states to ProgPy. Discrete states are implemented by initializing a state using the Discrete State Object. Created a TankModel to represent this.

This required some extra logic to support, especially changing the way containers work

I also added a jupyter notebook to demonstrate this feature. This notebook will later be merged into the numbered notebooks

Copy link

Thank you for opening this PR. Each PR into dev requires a code review. For the code review, look at the following:

  • Reviewer (someone other than author) should look for bugs, efficiency, readability, testing, and coverage in examples (if relevant).
  • Ensure that each PR adding a new feature should include a test verifying that feature.
  • All errors from static analysis must be resolved.
  • Review the test coverage reports (if there is a change) - will be added as comment on PR if there is a change
  • Review the software benchmarking results (if there is a change) - will be added as comment on PR
  • Any added dependencies are included in requirements.txt, setup.py, and dev_guide.rst (this document)
  • All warnings from static analysis must be reviewed and resolved - if deemed appropriate.

Copy link

@lymichelle21 lymichelle21 left a comment

Choose a reason for hiding this comment

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

As mentioned during the PR review meeting, adding a note on what the expected behavior should be for the sequential discrete states if the first state is 0 and we subtract from it (should it throw an error, stay at 0, or loop circularly to the highest state?)

@teubert teubert requested review from chetankul and kjjarvis March 3, 2025 19:37
Copy link

@lymichelle21 lymichelle21 left a comment

Choose a reason for hiding this comment

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

Jupyter notebook runs and examples all make sense - excited to see discrete states added to ProgPy.

Copy link
Contributor

@kjjarvis kjjarvis left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions and a question about the small_rotorcraft.py changes. Also, I'm getting a failed test on line 49 of test_discrete_states.py. Sounds like @lymichelle21 didn't get the same error so let's debug together on Wednesday

@lymichelle21
Copy link

lymichelle21 commented Mar 11, 2025

Just a few minor suggestions and a question about the small_rotorcraft.py changes. Also, I'm getting a failed test on line 49 of test_discrete_states.py. Sounds like @lymichelle21 didn't get the same error so let's debug together on Wednesday

Ah I see the error in test_discrete_states.py now - I had assumed that it was part of the collective python -m tests run and those ran successfully. It might be good to add the discrete states test to tests/__main__.py.

This is the following error with the sequential transition lower bound (and similar upper bound) test. This is likely because the current implementation doesn't raise a value error when hitting the bounds (as demonstrated in discrete_states.ipynb, it would just go to the bound directly).

Testing Discrete States
..F.
======================================================================
FAIL: test_sequential_transition (__main__.TestDiscreteStates.test_sequential_transition)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mly1/Documents/progpy/tests/test_discrete_state.py", line 49, in test_sequential_transition
    with self.assertRaises(ValueError):
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: ValueError not raised

----------------------------------------------------------------------
Ran 4 tests in 0.002s

FAILED (failures=1)
Traceback (most recent call last):
  File "/Users/mly1/Documents/progpy/tests/test_discrete_state.py", line 136, in <module>
    main()
  File "/Users/mly1/Documents/progpy/tests/test_discrete_state.py", line 133, in main
    raise Exception("Failed test")
Exception: Failed test

@lymichelle21
Copy link

Tests all run successfully :)

@lymichelle21 lymichelle21 self-requested a review March 17, 2025 18:27
Copy link

@lymichelle21 lymichelle21 left a comment

Choose a reason for hiding this comment

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

LGTM

@kjjarvis kjjarvis self-requested a review April 2, 2025 19:43
Copy link
Contributor

@kjjarvis kjjarvis left a comment

Choose a reason for hiding this comment

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

Looks good!

@teubert teubert merged commit ece64aa into dev Apr 2, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants