Visualise atom trace #657

MBartkowiakSTFC · 2025-02-05T10:48:58Z

Description of work
This PR adds the option of visualising the 'atom trace' in the 3D view. It is intended to show the accumulated volume that was taken by an atom within the entire time of the simulation. This may be useful for identifying hopping paths in porous materials, ionic conductors, etc.

Fixes

Adapted the code from Eric's molecular viewer.
Enabled multiple surfaces in a single view.
Reimplemented the trace calculation method to limit the grid size and account for the atom radius.

To test

Load a trajectory.
Calculate the trace for several atoms.
Check that the isosurface does not disappear when switching to a different frame.
Check that the isosurface disappears when switching to a different trajectory.

…-atom-trace

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/MolecularViewer.py

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/TraceWidget.py

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

…to maciej/reactivate-atom-trace

…SNeutronMuon/MDANSE into maciej/reactivate-atom-trace

MBartkowiakSTFC · 2025-02-10T11:34:30Z

There are two features missing in this PR:

There is no way to indicate to the user in the 3D view which atom the trace will be calculated for.
The colour input field has no validation.
This will be summarised in an issue and added in a later PR.

oerc0122 · 2025-02-10T13:51:34Z

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

@@ -115,7 +134,7 @@ def __getitem__(self, frame):
        except:
            conv_factor = 1.0
        else:
-            if pos_unit == "Ang":
+            if pos_unit == "Ang" or pos_unit == "Angstrom":


Suggested change

if pos_unit == "Ang" or pos_unit == "Angstrom":

if pos_unit in ("Ang", "Angstrom"):

These changes are part of PR #655

oerc0122 · 2025-02-10T13:51:50Z

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

@@ -193,7 +212,7 @@ def coordinates(self, frame):
        except:
            conv_factor = 1.0
        else:
-            if pos_unit == "Ang":
+            if pos_unit == "Ang" or pos_unit == "Angstrom":


Suggested change

if pos_unit == "Ang" or pos_unit == "Angstrom":

if pos_unit in ("Ang", "Angstrom"):

oerc0122 · 2025-02-10T13:52:01Z

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

        else:
-            if box_unit == "Ang":
+            if box_unit == "Ang" or box_unit == "Angstrom":


Suggested change

if box_unit == "Ang" or box_unit == "Angstrom":

if pos_unit in ("Ang", "Angstrom"):

oerc0122 · 2025-02-10T13:53:26Z

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

+                    if cell.shape == (3, 3):
+                        temp_array = np.array(cell)
+                    elif cell.shape == (3,):
+                        temp_array = np.diag(cell)


N.B. L. 288-289, can also use L. 280-281.

oerc0122 · 2025-02-10T13:53:57Z

MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

@@ -70,10 +88,10 @@ def __init__(self, h5_filename: Union[Path, str]):
        coords = self._h5_file["/particles/all/position/value"][0, :, :]
        try:
            pos_unit = self._h5_file["/particles/all/position/value"].attrs["unit"]
-        except:
+        except Exception:


Suggested change

except Exception:

except KeyError:

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/Controls.py

oerc0122 · 2025-02-10T14:04:53Z

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/MolecularViewer.py

-    ny = data.shape[1]
-    nz = data.shape[2]
+def array_to_3d_imagedata(data: np.ndarray, spacing: Tuple[float]):
+    nx, ny, nz = data.shape
    image = vtk.vtkImageData()
    image.SetDimensions(nx, ny, nz)
    dx, dy, dz = spacing


I realise it's not really part of this PR, but for later reference, l. 57-60 could be:

for (i, j, k), val in np.ndenumerate(data): image.SetScalarComponentFromDouble(i, j, k, 0, val)

or even

for loc, val in np.ndenumerate(data): image.SetScalarComponentFromDouble(*loc, 0, val)

OK, the code now uses np.ndenumerate, the former implementation.

oerc0122 · 2025-02-10T14:06:41Z

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/MolecularViewer.py

-        self._surface.PickableOff()
+        new_surface = vtk.vtkActor()
+        new_surface.SetMapper(mapper)
+        new_surface.GetProperty().SetColor((r, g, b))


Is there a reason to unpack the tuple only to repack it here?

I added input validation in the widget to make sure the user can only input a tuple of 3 numbers, and removed unpacking from this part of the code.

oerc0122 · 2025-02-10T14:08:26Z

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/TraceWidget.py

+        for label, widget in [
+            ("Remove the surface with index: ", self._surface_spinbox)
+        ]:
+            temp_box = QGroupBox(label, self)
+            temp_layout = QVBoxLayout(temp_box)
+            temp_layout.addWidget(widget)
+            layout.addWidget(temp_box)


Any reason for this to be split off?

The layout is populated sequentially. I needed to add a button, which is not wrapped in a QGroupBox, before this input field, but the logic of adding the input fields was the same before and after the button.

oerc0122 · 2025-02-10T14:09:41Z

MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/TraceWidget.py

+        params = {
+            "atom_number": 0,
+            "fine_sampling": 3,
+            "surface_colour": (0, 0.5, 0.75),


Does this value want to be a top-level constant somewherE?

Now this dictionary is a top-level constant and is used both in TraceWidget.py and MolecularViewer.py.

…to maciej/reactivate-atom-trace

MBartkowiakSTFC · 2025-02-19T10:10:39Z

I think that I addressed the points dealing with the 3D view and isosurface calculation. Some other comments about H5MD were covered by #655

Also, I tried to remove unused code from the 3D viewer and add/improve docstrings.

Do you think that these changes could be merged?

…to maciej/reactivate-atom-trace

MBartkowiakSTFC added 15 commits November 5, 2024 17:37

Enable basic atom trace visualisation

bea97c7

Merge branch 'protos' into maciej/reactivate-atom-trace

f4ffba9

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

190abbb

…-atom-trace

Add a basic atom trace dialog

8b65f3c

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

5491657

…-atom-trace

Simplify molecular trace calculation

f68e524

Include atom volume in molecular trace

6caff25

Optimise trajectory reading for trace

6913e12

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

951b012

…-atom-trace

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

2985962

…-atom-trace

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

c3d204a

…-atom-trace

Turn trace dialog into a widget

07d618c

Merge branch 'maciej/simplify-chemical-system' into maciej/reactivate…

e73ebd6

…-atom-trace

Embed the trace creator in another tab

d65e73b

Remove unused code

b7b6336

MBartkowiakSTFC marked this pull request as draft February 5, 2025 10:59

oerc0122 reviewed Feb 5, 2025

View reviewed changes

MBartkowiakSTFC and others added 10 commits February 7, 2025 07:11

Update MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

ef55608

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Update MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

265e342

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Update MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

1948b8e

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Update MDANSE/Src/MDANSE/Trajectory/H5MDTrajectory.py

f6d7abb

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Update MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/MolecularViewer.py

a019a1d

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Update MDANSE_GUI/Src/MDANSE_GUI/MolecularViewer/TraceWidget.py

a5b8606

Co-authored-by: Jacob Wilkins <[email protected]> Signed-off-by: Maciej Bartkowiak <[email protected]>

Improve code clarity following review comments

48ba26d

Merge branch 'protos' of https://github.com/ISISNeutronMuon/MDANSE in…

351df0f

…to maciej/reactivate-atom-trace

Merge branch 'maciej/reactivate-atom-trace' of https://github.com/ISI…

337e34b

…SNeutronMuon/MDANSE into maciej/reactivate-atom-trace

Raise an exception on wrong cell shape in H5MD trajectory

2ad58df

MBartkowiakSTFC requested a review from ChiCheng45 February 10, 2025 11:25

MBartkowiakSTFC marked this pull request as ready for review February 10, 2025 11:32

oerc0122 reviewed Feb 10, 2025

View reviewed changes

oerc0122 mentioned this pull request Feb 11, 2025

Improve H5MD trajectory support #655

Merged

MBartkowiakSTFC added 7 commits February 17, 2025 12:02

Merge branch 'protos' of https://github.com/ISISNeutronMuon/MDANSE in…

4244dda

…to maciej/reactivate-atom-trace

Add input validation for isosurface colour

570c1a1

Implement changes following a review

cd008a4

Merge branch 'protos' of https://github.com/ISISNeutronMuon/MDANSE in…

1fb24a5

…to maciej/reactivate-atom-trace

Add docstrings to the TraceWidget

1d428a1

Clean up MolecularViewer code

8388190

Clean up MolecularViewer code, remove unused methods

45297cf

MBartkowiakSTFC added 3 commits February 19, 2025 12:01

Merge branch 'protos' of https://github.com/ISISNeutronMuon/MDANSE in…

4b69c34

…to maciej/reactivate-atom-trace

Merge branch 'protos' of https://github.com/ISISNeutronMuon/MDANSE in…

917df66

…to maciej/reactivate-atom-trace

Respond to review comments

432881e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualise atom trace #657

Visualise atom trace #657

MBartkowiakSTFC commented Feb 5, 2025

MBartkowiakSTFC commented Feb 10, 2025

oerc0122 Feb 10, 2025

MBartkowiakSTFC Feb 10, 2025

oerc0122 Feb 10, 2025

oerc0122 Feb 10, 2025

oerc0122 Feb 10, 2025

oerc0122 Feb 10, 2025

oerc0122 Feb 10, 2025

MBartkowiakSTFC Feb 17, 2025

oerc0122 Feb 10, 2025

MBartkowiakSTFC Feb 17, 2025

oerc0122 Feb 10, 2025

MBartkowiakSTFC Feb 17, 2025

oerc0122 Feb 10, 2025

MBartkowiakSTFC Feb 17, 2025

MBartkowiakSTFC commented Feb 19, 2025

	if pos_unit == "Ang" or pos_unit == "Angstrom":
	if pos_unit in ("Ang", "Angstrom"):

	if box_unit == "Ang" or box_unit == "Angstrom":
	if pos_unit in ("Ang", "Angstrom"):

Visualise atom trace #657

Are you sure you want to change the base?

Visualise atom trace #657

Conversation

MBartkowiakSTFC commented Feb 5, 2025

MBartkowiakSTFC commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBartkowiakSTFC commented Feb 19, 2025