-
-
Notifications
You must be signed in to change notification settings - Fork 10
Implement continuous simulation loop #248
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { action, createStore } from 'easy-peasy'; | ||
| import type { SimulationModel, Simulation } from './simulation'; | ||
| import type { LammpsWeb } from '../types'; | ||
|
|
||
| describe('SimulationModel', () => { | ||
| describe('finished state', () => { | ||
| it('should initialize with finished as false', () => { | ||
| // Arrange | ||
| const store = createStore<Pick<SimulationModel, 'finished' | 'setFinished'>>({ | ||
| finished: false, | ||
| setFinished: action((state, value: boolean) => { | ||
| state.finished = value; | ||
| }), | ||
| }); | ||
|
|
||
| // Act | ||
| const state = store.getState(); | ||
|
|
||
| // Assert | ||
| expect(state.finished).toBe(false); | ||
| }); | ||
|
|
||
| it('should set finished state to true', () => { | ||
| // Arrange | ||
| const store = createStore<Pick<SimulationModel, 'finished' | 'setFinished'>>({ | ||
| finished: false, | ||
| setFinished: action((state, value: boolean) => { | ||
| state.finished = value; | ||
| }), | ||
| }); | ||
|
|
||
| // Act | ||
| store.getActions().setFinished(true); | ||
|
|
||
| // Assert | ||
| expect(store.getState().finished).toBe(true); | ||
| }); | ||
|
|
||
| it('should set finished state to false', () => { | ||
| // Arrange | ||
| const store = createStore<Pick<SimulationModel, 'finished' | 'setFinished'>>({ | ||
| finished: true, | ||
| setFinished: action((state, value: boolean) => { | ||
| state.finished = value; | ||
| }), | ||
| }); | ||
|
|
||
| // Act | ||
| store.getActions().setFinished(false); | ||
|
|
||
| // Assert | ||
| expect(store.getState().finished).toBe(false); | ||
| }); | ||
|
|
||
| it('should reset finished state on reset action', () => { | ||
| // Arrange | ||
| const store = createStore<Pick<SimulationModel, 'finished' | 'files' | 'lammps' | 'reset'>>({ | ||
| finished: true, | ||
| files: ['test.in'], | ||
| lammps: undefined, | ||
| reset: action((state) => { | ||
| state.files = []; | ||
| state.lammps = undefined; | ||
| state.finished = false; | ||
| }), | ||
| }); | ||
|
|
||
| // Act | ||
| store.getActions().reset(undefined); | ||
|
|
||
| // Assert | ||
| expect(store.getState().finished).toBe(false); | ||
| expect(store.getState().files).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('continueSimulation types', () => { | ||
| it('should accept undefined timesteps parameter', () => { | ||
| // This test verifies that the type signature allows undefined | ||
| // The actual thunk implementation is tested through integration tests | ||
|
|
||
| // Arrange | ||
| type ContinueSimulationAction = (timesteps: number | undefined) => void; | ||
|
|
||
| // Act & Assert - This should compile without errors | ||
| const mockAction: ContinueSimulationAction = (timesteps) => { | ||
| if (timesteps === undefined) { | ||
| // Default behavior | ||
| } else { | ||
| // Custom timesteps | ||
| } | ||
| }; | ||
|
|
||
| // Verify both call signatures work | ||
| mockAction(1000); | ||
| mockAction(undefined); | ||
|
|
||
| expect(mockAction).toBeDefined(); | ||
| }); | ||
| }); | ||
|
Comment on lines
+78
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case only verifies that the type signature of a mock action allows While type checking is useful, a unit test should ideally verify some logic. This test as it stands provides little value and could be misleading. I'd recommend either:
|
||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -40,6 +40,7 @@ export interface Simulation { | |||||
| export interface SimulationModel { | ||||||
| running: boolean; | ||||||
| paused: boolean; | ||||||
| finished: boolean; | ||||||
| showConsole: boolean; | ||||||
| simulation?: Simulation; | ||||||
| files: string[]; | ||||||
|
|
@@ -50,6 +51,7 @@ export interface SimulationModel { | |||||
| addLammpsOutput: Action<SimulationModel, string>; | ||||||
| setShowConsole: Action<SimulationModel, boolean>; | ||||||
| setPaused: Action<SimulationModel, boolean>; | ||||||
| setFinished: Action<SimulationModel, boolean>; | ||||||
| setCameraPosition: Action<SimulationModel, THREE.Vector3 | undefined>; | ||||||
| setCameraTarget: Action<SimulationModel, THREE.Vector3 | undefined>; | ||||||
| setSimulation: Action<SimulationModel, Simulation>; | ||||||
|
|
@@ -60,6 +62,7 @@ export interface SimulationModel { | |||||
| syncFilesWasm: Thunk<SimulationModel, string | undefined>; | ||||||
| syncFilesJupyterLite: Thunk<SimulationModel, undefined>; | ||||||
| run: Thunk<SimulationModel>; | ||||||
| continueSimulation: Thunk<SimulationModel, number | undefined>; | ||||||
| newSimulation: Thunk<SimulationModel, Simulation>; | ||||||
| lammps?: LammpsWeb; | ||||||
| reset: Action<SimulationModel, undefined>; | ||||||
|
|
@@ -68,6 +71,7 @@ export interface SimulationModel { | |||||
| export const simulationModel: SimulationModel = { | ||||||
| running: false, | ||||||
| paused: false, | ||||||
| finished: false, | ||||||
| showConsole: false, | ||||||
| files: [], | ||||||
| lammpsOutput: [], | ||||||
|
|
@@ -83,6 +87,9 @@ export const simulationModel: SimulationModel = { | |||||
| setPaused: action((state, value: boolean) => { | ||||||
| state.paused = value; | ||||||
| }), | ||||||
| setFinished: action((state, value: boolean) => { | ||||||
| state.finished = value; | ||||||
| }), | ||||||
| setCameraPosition: action((state, cameraPosition: THREE.Vector3) => { | ||||||
| state.cameraPosition = cameraPosition; | ||||||
| }), | ||||||
|
|
@@ -318,6 +325,7 @@ export const simulationModel: SimulationModel = { | |||||
| allActions.simulationStatus.reset(); | ||||||
| actions.setShowConsole(false); | ||||||
| actions.resetLammpsOutput(); | ||||||
| actions.setFinished(false); | ||||||
|
|
||||||
| await actions.syncFilesWasm(undefined); | ||||||
|
|
||||||
|
|
@@ -389,6 +397,7 @@ export const simulationModel: SimulationModel = { | |||||
| // Simulation got canceled. | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(false); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "canceled", | ||||||
|
|
@@ -401,6 +410,7 @@ export const simulationModel: SimulationModel = { | |||||
| }); | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(false); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "failed", | ||||||
|
|
@@ -412,6 +422,7 @@ export const simulationModel: SimulationModel = { | |||||
| allActions.processing.runPostTimestep(true); | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(true); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "completed", | ||||||
|
|
@@ -421,6 +432,106 @@ export const simulationModel: SimulationModel = { | |||||
| actions.syncFilesJupyterLite(); | ||||||
| allActions.simulationStatus.setLastCommand(undefined); | ||||||
| }), | ||||||
| continueSimulation: thunk( | ||||||
| async (actions, timesteps: number | undefined, { getStoreState, getStoreActions }) => { | ||||||
| // @ts-ignore | ||||||
| const simulation = getStoreState().simulation.simulation as Simulation; | ||||||
| if (!simulation) { | ||||||
| return; | ||||||
| } | ||||||
| // @ts-ignore | ||||||
| const lammps = getStoreState().simulation.lammps as LammpsWeb; | ||||||
|
Comment on lines
+437
to
+443
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of |
||||||
| if (!lammps || lammps.getIsRunning()) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const allActions = getStoreActions() as Actions<StoreModel>; | ||||||
|
|
||||||
| actions.setShowConsole(false); | ||||||
| actions.setFinished(false); | ||||||
|
|
||||||
| lammps.start(); | ||||||
| actions.setRunning(true); | ||||||
| track("Simulation.Continue", { | ||||||
| simulationId: simulation?.id, | ||||||
| timesteps: timesteps, | ||||||
| ...getEmbeddingParams() | ||||||
| }); | ||||||
| time_event("Simulation.Stop"); | ||||||
|
|
||||||
| let errorMessage: string | undefined = undefined; | ||||||
| const startTime = performance.now(); | ||||||
|
|
||||||
| try { | ||||||
| // Use the suggested command format: run with pre no post no | ||||||
| // This continues the simulation without re-initializing | ||||||
| const runCommand = timesteps ? `run ${timesteps} pre no post no` : `run 1000 pre no post no`; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The number
Suggested change
|
||||||
| lammps.runCommand(runCommand); | ||||||
| } catch (exception: any) { | ||||||
| console.log("Got exception: ", exception); | ||||||
| errorMessage = lammps.getExceptionMessage(exception); | ||||||
| console.log("Got error running LAMMPS: ", errorMessage); | ||||||
| } | ||||||
|
|
||||||
| if (!errorMessage) { | ||||||
| errorMessage = lammps.getErrorMessage(); | ||||||
| } | ||||||
|
|
||||||
| // @ts-ignore | ||||||
| const computes = getStoreState().simulationStatus.computes as Compute[]; | ||||||
|
|
||||||
| const endTime = performance.now(); | ||||||
| const duration = (endTime - startTime) / 1000; // seconds | ||||||
| const metricsData = { | ||||||
| timesteps: lammps.getTimesteps(), | ||||||
| timestepsPerSecond: (lammps.getTimesteps() / duration).toFixed(3), | ||||||
| numAtoms: lammps.getNumAtoms(), | ||||||
| computes: Object.keys(computes), | ||||||
| }; | ||||||
| actions.setPaused(false); | ||||||
|
|
||||||
| if (errorMessage) { | ||||||
| if (errorMessage.includes("Atomify::canceled")) { | ||||||
| allActions.processing.runPostTimestep(true); | ||||||
| // Simulation got canceled. | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(false); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "canceled", | ||||||
| ...metricsData, | ||||||
| }); | ||||||
| } else { | ||||||
| notification.error({ | ||||||
| message: errorMessage, | ||||||
| duration: 5, | ||||||
| }); | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(false); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "failed", | ||||||
| errorMessage, | ||||||
| ...metricsData, | ||||||
| }); | ||||||
| } | ||||||
| } else { | ||||||
| allActions.processing.runPostTimestep(true); | ||||||
| actions.setRunning(false); | ||||||
| actions.setShowConsole(true); | ||||||
| actions.setFinished(true); | ||||||
| track("Simulation.Stop", { | ||||||
| simulationId: simulation?.id, | ||||||
| stopReason: "completed", | ||||||
| ...metricsData, | ||||||
| }); | ||||||
| } | ||||||
| actions.syncFilesJupyterLite(); | ||||||
| allActions.simulationStatus.setLastCommand(undefined); | ||||||
| }, | ||||||
| ), | ||||||
|
Comment on lines
+435
to
+534
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a large block of code for handling simulation completion, cancellation, and failure (lines 480-532) that is nearly identical to the logic in the I recommend refactoring this common logic into a separate helper function. This function could take parameters like the error message and metrics data, and then handle setting the state and tracking events accordingly. This would significantly improve code clarity and maintainability. |
||||||
| newSimulation: thunk( | ||||||
| async ( | ||||||
| actions, | ||||||
|
|
@@ -517,5 +628,6 @@ export const simulationModel: SimulationModel = { | |||||
| reset: action((state) => { | ||||||
| state.files = []; | ||||||
| state.lammps = undefined; | ||||||
| state.finished = false; | ||||||
| }), | ||||||
| }; | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are not testing the actual implementation of the
setFinishedandresetactions fromsimulationModel. Instead, they re-implement the actions within thecreateStorecall for each test. This means you are testing a mock implementation, not your production code.To fix this, you should import the
simulationModeland use it to create the store for your tests. This will ensure you are testing the real actions.Here's an example of how you could restructure the tests: