Skip to content

refactor: Refactor sync modifiers to use shared Data1D processing logic #193

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.

Sign up for GitHub

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

Draft
andeplane wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

refactor: Refactor sync modifiers to use shared Data1D processing logic #193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3093ce
refactor: Refactor sync modifiers to use shared Data1D processing logic
cursoragent Nov 1, 2025
d13436e
refactor: Address PR review comments for processData1D function
cursoragent Nov 1, 2025
47e8f17
refactor: Improve readability of data processing loop in processData1D
cursoragent Nov 1, 2025
f9fc7ef
Merge branch 'main' into cursor/solve-issue-186-and-create-pr-df44
andeplane Nov 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions src/modifiers/modifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,95 @@
import { ModifierInput, ModifierOutput } from "./types";
import { LMPModifier, Data1D } from "../types";

interface ModifierProps {
name: string;
active: boolean;
}

/**
* Shared function to process Data1D from LMPModifier.
* Handles Data1D extraction, processing, and WASM memory cleanup.
*/
export function processData1D(
lmpModifier: LMPModifier,
data1D: Data1D | undefined,
input: ModifierInput,
everything: boolean,
syncDataPoints: boolean,
clearPerSync: boolean,
): { data1D: Data1D | undefined; hasData1D: boolean } {
// Get data1DNamesWrapper and extract size, then delete immediately
const data1DNamesWrapper = lmpModifier.getData1DNames();
const hasData1D = data1DNamesWrapper.size() > 0;
const data1DNamesSize = data1DNamesWrapper.size();
data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak

if (data1DNamesSize === 0) {
return { data1D, hasData1D: false };
}

// Initialize data1D if needed
let currentData1D = data1D;
if (currentData1D == null) {
currentData1D = {
data: [],
labels: [],
};
}

if ((everything || syncDataPoints) && currentData1D) {
// Data points is only for plotting figures
if (clearPerSync) {
// For histograms (compute rdf etc) we don't have time as x axis, so we clear every time
currentData1D.data = [];
}

const lengthBeforeWeStart = currentData1D.data.length; // Used to avoid copying all data every time

if (currentData1D.labels.length === 0) {
// First label is never visible
currentData1D.labels.push("x");
}

// Get data1DVector once before the loop for better performance
const data1DVector = lmpModifier.getData1D();

for (let j = 0; j < data1DNamesSize; j++) {
const lmpData = data1DVector.get(j);

if (currentData1D.labels.length - 1 === j) {
// Add missing labels
currentData1D.labels.push(lmpData.getLabel());
}

const xValuesPointer = lmpData.getXValuesPointer() / 4;
const yValuesPointer = lmpData.getYValuesPointer() / 4;
const xValues = input.wasm.HEAPF32.subarray(
xValuesPointer,
xValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
const yValues = input.wasm.HEAPF32.subarray(
yValuesPointer,
yValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
currentData1D.data.push([xValues[k]]);
}
currentData1D.data[k].push(yValues[k]);
}
Comment on lines 75 to 81
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 1, 2025

Choose a reason for hiding this comment

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

medium

The current logic for populating currentData1D.data is functionally correct, but it could be made more explicit to improve readability. By using an if/else block, you can clearly separate the logic for initializing a new row (for j === 0) from appending to an existing row (for j > 0). This makes the intent of the code easier to grasp at a glance.

Suggested change
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
currentData1D.data.push([xValues[k]]);
}
currentData1D.data[k].push(yValues[k]);
}
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
currentData1D.data.push([xValues[k], yValues[k]]);
} else {
currentData1D.data[k].push(yValues[k]);
}
}


// Delete the Data1D copy to prevent memory leak
lmpData.delete();
}

// Delete the vector wrapper after the loop to prevent memory leak
data1DVector.delete();
}

return { data1D: currentData1D, hasData1D };
}

class Modifier {
public name: string;
public key: string;
Expand Down
80 changes: 13 additions & 67 deletions src/modifiers/synccomputesmodifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Modifier from "./modifier";
import { processData1D } from "./modifier";
import { ModifierInput, ModifierOutput } from "./types";

interface SyncComputesModifierProps {
Expand Down Expand Up @@ -51,73 +52,18 @@ class SyncComputesModifier extends Modifier {
compute.xLabel = compute.lmpCompute.getXLabel();
compute.yLabel = compute.lmpCompute.getYLabel();
compute.scalarValue = compute.lmpCompute.getScalarValue();

// Get data1DNamesWrapper and extract size, then delete immediately
const data1DNamesWrapper = compute.lmpCompute.getData1DNames();
compute.hasData1D = data1DNamesWrapper.size() > 0;
const data1DNamesSize = data1DNamesWrapper.size();
data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak

if (data1DNamesSize > 0) {
compute.clearPerSync = compute.lmpCompute.getClearPerSync();

if (compute.data1D == null) {
compute.data1D = {
data: [],
labels: [],
};
}

if ((everything || compute.syncDataPoints) && compute.data1D) {
// Data points is only for plotting figures
if (compute.clearPerSync) {
// For histograms (compute rdf etc) we don't have time as x axis, so we clear every time
compute.data1D.data = [];
}

const lengthBeforeWeStart = compute.data1D.data.length; // Used to avoid coping all data every time

if (compute.data1D.labels.length === 0) {
// First label is never visible
compute.data1D.labels.push("x");
}

// Get data1DVector once before the loop for better performance
const data1DVector = compute.lmpCompute.getData1D();

for (let j = 0; j < data1DNamesSize; j++) {
const lmpData = data1DVector.get(j);

if (compute.data1D.labels.length - 1 === j) {
// Add missing labels
compute.data1D.labels.push(lmpData.getLabel());
}

const xValuesPointer = lmpData.getXValuesPointer() / 4;
const yValuesPointer = lmpData.getYValuesPointer() / 4;
const xValues = input.wasm.HEAPF32.subarray(
xValuesPointer,
xValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
const yValues = input.wasm.HEAPF32.subarray(
yValuesPointer,
yValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
compute.data1D.data.push([xValues[k]]);
}
compute.data1D.data[k].push(yValues[k]);
}

// Delete the Data1D copy to prevent memory leak
lmpData.delete();
}

// Delete the vector wrapper after the loop to prevent memory leak
data1DVector.delete();
}
}
compute.clearPerSync = compute.lmpCompute.getClearPerSync();

const { data1D, hasData1D } = processData1D(
compute.lmpCompute,
compute.data1D,
input,
everything,
compute.syncDataPoints,
compute.clearPerSync,
);
compute.data1D = data1D;
compute.hasData1D = hasData1D;
}
output.computes[name] = compute;
}
Expand Down
76 changes: 12 additions & 64 deletions src/modifiers/syncfixesmodifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Modifier from "./modifier";
import { processData1D } from "./modifier";
import { ModifierInput, ModifierOutput } from "./types";

interface SyncFixesModifierProps {
Expand Down Expand Up @@ -48,72 +49,19 @@ class SyncFixesModifier extends Modifier {
fix.xLabel = fix.lmpFix.getXLabel();
fix.yLabel = fix.lmpFix.getYLabel();
fix.scalarValue = fix.lmpFix.getScalarValue();
fix.clearPerSync = fix.lmpFix.getClearPerSync();

// Get data1DNamesWrapper and extract size, then delete immediately
const data1DNamesWrapper = fix.lmpFix.getData1DNames();
fix.hasData1D = data1DNamesWrapper.size() > 0;
const data1DNamesSize = data1DNamesWrapper.size();
data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak
const { data1D, hasData1D } = processData1D(
fix.lmpFix,
fix.data1D,
input,
everything,
fix.syncDataPoints,
fix.clearPerSync,
);
fix.data1D = data1D;
fix.hasData1D = hasData1D;

if (data1DNamesSize > 0) {
fix.clearPerSync = fix.lmpFix.getClearPerSync();
if (fix.data1D == null) {
fix.data1D = {
data: [],
labels: [],
};
}

if ((everything || fix.syncDataPoints) && fix.data1D) {
// Data points is only for plotting figures
if (fix.clearPerSync) {
// For histograms (compute rdf etc) we don't have time as x axis, so we clear every time
fix.data1D.data = [];
}

const lengthBeforeWeStart = fix.data1D.data.length; // Used to avoid coping all data every time

if (fix.data1D.labels.length === 0) {
// First label is never visible
fix.data1D.labels.push("x");
}

// Get data1DVector once before the loop for better performance
const data1DVector = fix.lmpFix.getData1D();

for (let j = 0; j < data1DNamesSize; j++) {
const lmpData = data1DVector.get(j);

if (fix.data1D.labels.length - 1 === j) {
// Add missing labels
fix.data1D.labels.push(lmpData.getLabel());
}

const xValuesPointer = lmpData.getXValuesPointer() / 4;
const yValuesPointer = lmpData.getYValuesPointer() / 4;
const xValues = input.wasm.HEAPF32.subarray(
xValuesPointer,
xValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
const yValues = input.wasm.HEAPF32.subarray(
yValuesPointer,
yValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
fix.data1D.data.push([xValues[k]]);
}
fix.data1D.data[k].push(yValues[k]);
}

// Delete the Data1D copy to prevent memory leak
lmpData.delete();
}

// Delete the vector wrapper after the loop to prevent memory leak
data1DVector.delete();
}
}
output.fixes[name] = fix;
}
};
Expand Down
76 changes: 12 additions & 64 deletions src/modifiers/syncvariablesmodifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Modifier from "./modifier";
import { processData1D } from "./modifier";
import { ModifierInput, ModifierOutput } from "./types";

interface SyncVariablesModifierProps {
Expand Down Expand Up @@ -50,72 +51,19 @@ class SyncVariablesModifier extends Modifier {
variable.yLabel = variable.lmpVariable.getYLabel();
variable.scalarValue = variable.lmpVariable.getScalarValue();
variable.hasScalarData = variable.lmpVariable.hasScalarData();
variable.clearPerSync = variable.lmpVariable.getClearPerSync();

// Get data1DNamesWrapper and extract size, then delete immediately
const data1DNamesWrapper = variable.lmpVariable.getData1DNames();
variable.hasData1D = data1DNamesWrapper.size() > 0;
const data1DNamesSize = data1DNamesWrapper.size();
data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak

if (data1DNamesSize > 0) {
variable.clearPerSync = variable.lmpVariable.getClearPerSync();
if (variable.data1D == null) {
variable.data1D = {
data: [],
labels: [],
};
}
const { data1D, hasData1D } = processData1D(
variable.lmpVariable,
variable.data1D,
input,
everything,
variable.syncDataPoints,
variable.clearPerSync,
);
variable.data1D = data1D;
variable.hasData1D = hasData1D;

if ((everything || variable.syncDataPoints) && variable.data1D) {
// Data points is only for plotting figures
if (variable.clearPerSync) {
// For histograms (compute rdf etc) we don't have time as x axis, so we clear every time
variable.data1D.data = [];
}

const lengthBeforeWeStart = variable.data1D.data.length; // Used to avoid coping all data every time

if (variable.data1D.labels.length === 0) {
// First label is never visible
variable.data1D.labels.push("x");
}

// Get data1DVector once before the loop for better performance
const data1DVector = variable.lmpVariable.getData1D();

for (let j = 0; j < data1DNamesSize; j++) {
const lmpData = data1DVector.get(j);

if (variable.data1D.labels.length - 1 === j) {
// Add missing labels
variable.data1D.labels.push(lmpData.getLabel());
}

const xValuesPointer = lmpData.getXValuesPointer() / 4;
const yValuesPointer = lmpData.getYValuesPointer() / 4;
const xValues = input.wasm.HEAPF32.subarray(
xValuesPointer,
xValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
const yValues = input.wasm.HEAPF32.subarray(
yValuesPointer,
yValuesPointer + lmpData.getNumPoints(),
) as Float32Array;
for (let k = lengthBeforeWeStart; k < xValues.length; k++) {
if (j === 0) {
variable.data1D.data.push([xValues[k]]);
}
variable.data1D.data[k].push(yValues[k]);
}

// Delete the Data1D copy to prevent memory leak
lmpData.delete();
}

// Delete the vector wrapper after the loop to prevent memory leak
data1DVector.delete();
}
}
output.variables[name] = variable;
}
};
Expand Down