Add spec test for pumping example

[mohsen-ghaffari1992]

I have written some tests for pumping. Please let me know, if you had other ideas or expect different tests.

Fixes #145.

[wasowski]

hi @mohsen-ghaffari1992 - is this waiting for my review, or is there more to be done here?

[mohsen-ghaffari1992]

hi @mohsen-ghaffari1992 - is this waiting for my review, or is there more to be done here?

Hi @wasowski, yes please.

[wasowski]

@mohsen-ghaffari1992 , I did some cleanup. I was getting some failed tests with PumpSpec (two more precisely). I added assertions to PumpState and PumpObservableState (as I also did in the CartPole, and now I am getting crashes with negative flow or too high tank level. Perhaps I added too many assertions, but please check. Weaken the assertions that are too strong, and investigate why the tests fail. Thanks.

Note that I rebased, so you need to pull the branch before starting.

[wasowski]

Hmmm, I have rebased on "modern" symsim. Now trying testOnly symsim.examples.cocnrete.pumping.PumpIsAgentSpec and the test is hanging. Shouldn't this be fast? Do you know what is going on @mohsen-ghaffari1992 ? Are you seeing the same thing ?

EDIT: It did not hang. It just took 2 minutes to complete :) And then I got this error:

sbt:symsim> testOnly symsim.examples.concrete.pumping.PumpIsAgentSpec
failing seed for agent.observe (step (s) (a)._1) ∈ ObservableState is gEZdVE2-ieeNYm8Aw0VfSxACr-Zj3vmCyXZzFS011UA=
failing seed for agent.∀ s ∈ initialize ⋅ ∀ a ∈ Actions ⋅ step (s) (a) ≠ (s,_) is kio6VNoVBS33Mem6aB547rWbEQ1YHi8N6g8fdgOhh9H=
failing seed for episodic agent.terminates before TimeHorizon is WtGDHKjiijFIYmNYqyEcKTY26EBO54GYanjUnmM2RAL=
[info] PumpIsAgentSpec:
[info] - concrete.pumping.Pump is an Agent.agent.observe (initialize) ∈ ObservableState (21 seconds, 664 milliseconds)
[info] - concrete.pumping.Pump is an Agent.agent.observe (s) ∈ ObservableState (1 minute, 20 seconds)
[info] - concrete.pumping.Pump is an Agent.agent.observe (step (s) (a)._1) ∈ ObservableState *** FAILED *** (221 milliseconds)
[info]   IllegalArgumentException was thrown during property evaluation.
[info]     Message: requirement failed: Water level is too low: -0.0563905780784395
[info]     Occurred when passed generated values (
[info]       arg0 = [flow=2.4423356127739293, head=9.16886934304885, head mean=8.106189210032788, tank level=7.7737703074896025, time=17, water=0.4120533209622866, past head means=List(8.235386369530163, 10.075787471093513, 7.8937450811433365, 8.372206918704428, 7.91491049774692)],
[info]       arg1 = 85.0
[info]     )
[info]   Init Seed: 5056118899236358154
[info] - concrete.pumping.Pump is an Agent.agent.¬isFinal (initialize) (15 milliseconds)
[info] - concrete.pumping.Pump is an Agent.agent.∀ s ∈ initialize ⋅ ∀ a ∈ Actions ⋅ step (s) (a) ≠ (s,_) *** FAILED *** (15 milliseconds)
[info]   IllegalArgumentException was thrown during property evaluation.
[info]     Message: requirement failed: Water level is too high: 9.77604519673651
[info]     Occurred when passed generated values (
[info]       arg0 = [flow=80.0, head=10.0, head mean=10.0, tank level=1000.0, time=0, water=9.11, past head means=List(10.0, 10.0, 10.0, 10.0, 10.0)],
[info]       arg1 = 50.0
[info]     )
[info]   Init Seed: -1037815395271901692
[info] - concrete.pumping.Pump is Episodic.episodic agent.terminates before TimeHorizon *** FAILED *** (96 milliseconds)
[info]   IllegalArgumentException was thrown during property evaluation.
[info]     Message: requirement failed: Head is too high: 11.053850064658455
[info]     Occurred when passed generated values (
[info]       arg0 = [flow=80.0, head=10.0, head mean=10.0, tank level=1000.0, time=0, water=9.11, past head means=List(10.0, 10.0, 10.0, 10.0, 10.0)],
[info]       arg1 = LazyList(70.0, 70.0, <not computed>)
[info]     )
[info]   Init Seed: -3859500586003490784
[info] Run completed in 1 minute, 44 seconds.
[info] Total number of tests run: 6
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 3, failed 3, canceled 0, ignored 0, pending 0
[info] *** 3 TESTS FAILED ***

I am starting to dig into this, but if you know what is going on, or what is the status of this -> let me know.

[mohsen-ghaffari1992]

I have just tested it, passes! [A black screen with white text Description automatically generated] From: Andrzej Wąsowski ***@***.***> Date: Wednesday, 29 November 2023 at 09.54 To: itu-square/symsim ***@***.***> Cc: Mohsen Ghaffari ***@***.***>, Mention ***@***.***> Subject: Re: [itu-square/symsim] Add spec test for pumping example (PR #163) Hmmm, I have rebased on "modern" symsim. Now trying testOnly symsim.examples.cocnrete.pumping.PumpIsAgentSpec and the test is hanging. Shouldn't this be fast? Do you know what is going on @mohsen-ghaffari1992<https://github.com/mohsen-ghaffari1992> ? Are you seeing the same thing ? — Reply to this email directly, view it on GitHub<#163 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ASQKWRMFZMUBDDHMEPUSVP3YG3Z5HAVCNFSM6AAAAAARO6ID5WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRGQ3TIOBWGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[wasowski]

Hmm ... This is really strange - how it can pass on your machine and fail on mine. Have you pulled? (The CI seems to be hanging on it, too). I am just trying the PumpIsSpec and this one is failing on 3 tests, repeatedly.

[mohsen-ghaffari1992]

I cloned the repo right after your mail. What I always am doing for testing symsim is adding the memory: export SBT_OPTS="-Xmx3G -XX:+UseG1GC -Xss2M" From: Andrzej Wąsowski ***@***.***> Date: Wednesday, 29 November 2023 at 10.10 To: itu-square/symsim ***@***.***> Cc: Mohsen Ghaffari ***@***.***>, Mention ***@***.***> Subject: Re: [itu-square/symsim] Add spec test for pumping example (PR #163) Hmm ... This is really strange - how it can pass on your machine and fail on mine. Have you pulled? (The CI seems to be hanging on it, too). I am just trying the PumpIsSpec and this one is failing on 3 tests, repeatedly. — Reply to this email directly, view it on GitHub<#163 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ASQKWRM7NIMG26I3WOMJCMTYG33ZRAVCNFSM6AAAAAARO6ID5WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRGQ4TSMZTGA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[wasowski]

So you say that testOnly symsim.examples.concrete.pumping.PumpIsAgentSpec on your clean clone does not fail any tests, and 6 tests pass?

(unfortunately, I am already using these options)

[wasowski]

I have changed AgentLaws to use a set instead of a list for observableStates. This speeds up two of the passing tests 1000 times for this model. But the failing ones remain unchanged. I pushed this change to this PR, so now we definitely wanna merge it. But before we merge, I need the tests to pass :)

[mohsen-ghaffari1992]

I have changed AgentLaws to use a set instead of a list for observableStates. This speeds up two of the passing tests 1000 times for this model. But the failing ones remain unchanged. I pushed this change to this PR, so now we definitely wanna merge it. But before we merge, I need the tests to pass :)

Now, I am failing in three tests as well as you!
I noticed in Agent we have the following

lazy val allObservableStates: List[ObservableState] =
    enumState.membersAscending.toList

Do not you think this can be the reason for current issue?

[wasowski]

Now, I am failing in three tests as well as you! I noticed in Agent we have the following

lazy val allObservableStates: List[ObservableState] =
    enumState.membersAscending.toList

Do not you think this can be the reason for current issue?

Well, I do not know how this could be a cause. I have an error that flow is too low, or water level is too high. I do not think these values can (obviously) change by doing a list conversion. It seems like the step function is crossing the max and min bounds for some values?

[mohsen-ghaffari1992]

You right. I should look into this example again. From: Andrzej Wąsowski ***@***.***> Date: Thursday, 30 November 2023 at 17.34 To: itu-square/symsim ***@***.***> Cc: Mohsen Ghaffari ***@***.***>, Mention ***@***.***> Subject: Re: [itu-square/symsim] Add spec test for pumping example (PR #163) Now, I am failing in three tests as well as you! I noticed in Agent we have the following lazy val allObservableStates: List[ObservableState] = enumState.membersAscending.toList Do not you think this can be the reason for current issue? Well, I do not know how this could be a cause. I have an error that flow is too low, or water level is too high. I do not think these values can (obviously) change by doing a list conversion. It seems like the step function is crossing the max and min bounds for some values? — Reply to this email directly, view it on GitHub<#163 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ASQKWRJS5AURXBFU44GU6RLYHCYR3AVCNFSM6AAAAAARO6ID5WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZUGEZDKMJYHA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[wasowski]

Could you check with Andreas, or in his Python code, whether the Min and Max values are meant to never be crossed, or whether the should be just reset to min/max (if crossed), or there is a state for all the values below the min and for all the states above the max? What did he do to never cross these extremes?

And are all the means and standard deviations for the three normal distributions in the step function the same there? In principle these Gaussians can produce arbitrarily high noise, so I would expect that the original implementation had to account for this somehow, but I do not have access to that code.

It could also be that crossing the extremes resulted in a failure (negative reward, terminal) state. If so, we may need to change our implementation to allow these states.

I found the original code here: https://github.com/andreashhpetersen/rl_pump_controller/blob/master/code/environment.py

Something interesting happens in line 61 - it appears that self.observation_space is a gym property that sets a bounding box on the state space. It is never used in this python file, AFAICS. So the semantics of this bounding box is likely given by the implementation of from the gym.