Repository navigation
Use configured observations in Pinball and fix render() - #363
Open
NopeCharon wants to merge 2 commits into
Open
NopeCharon wants to merge 2 commits into
NopeCharon wants to merge 2 commits into
Conversation
Pinball overrode get_observations() to always return the lift and drag coefficients, bypassing the ObservationFunction that FlowConfig builds from observation_type. num_outputs, and therefore the FlowEnv observation space, still followed observation_type, so with probe observations the environment returned 6 values for an observation space of 2 * len(probes) entries and failed gymnasium's env checker. Drop the override: the inherited FlowConfig.get_observations() already returns (CL1, CL2, CL3, CD1, CD2, CD3) for the default "lift_drag" observation, so the default behaviour is unchanged. Pinball.render() referred to self.flow.u, self.flow.pressure_space, self.flow.x0, self.flow.y0 and self.flow.rad, but Pinball is itself the flow and has no flow attribute, so every call raised AttributeError. Use the instance attributes, matching the other Firedrake flows, and convert the radius Constant to a float for matplotlib. Add tests for probe observations through FlowEnv and for render(), and correct the documented ordering of the Pinball lift_drag observation. Closes dynamicslab#361
This branch has not been deployed
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.
Closes #361
Changes
Pinball.get_observations()override. It always returned lift/drag and skipped theObservationFunctionthatFlowConfigbuilds fromobservation_type.num_outputs, and soFlowEnv.observation_space, still followedobservation_type, so probe observations came back with the wrong length. The inherited method returns the same(CL1, CL2, CL3, CD1, CD2, CD3)for the defaultlift_drag, so default behaviour doesn't change.Pinball.render()usedself.flow.*, butPinballis itself the flow and has noflowattribute. It now uses its own attributes, like the other Firedrake flows, and turns the radiusConstantinto a float for matplotlib.test_probe_observations(velocity probes throughFlowEnv) andtest_rendertotest/test_pinball.py.lift_dragobservation in the getting-started docs and README. They listed(CL1, CD1, CL2, CD2, CL3, CD3).Validation
All runs used the
firedrakeproject/firedrake-vanilla-defaultcontainer.main,test_probe_observationsfails ((6,) == (12,)), and so doestest_render(AttributeError: 'Pinball' object has no attribute 'flow'). Both pass with this change.gymnasium.utils.env_checker.check_envon a velocity-probe Pinball env: onmainit fails with "The first element returned byenv.reset()is not within the observation space". With this change it gets past the observation checks. It then stops on a separate check, becauseFlowEnv.reset()doesn't callsuper().reset(seed=seed). That's out of scope here.render()draws the vorticity field and all three cylinders in the right places.test_import_fine,test_steady,test_integrateandtest_controlpass.test_steady_rotation(wrong CL) andtest_env(nodtpassed toSemiImplicitBDF) fail the same way onmain.ruff check,ruff format --check,isort --check-onlyandcodespellpass.