MDANSE icon indicating copy to clipboard operation
MDANSE copied to clipboard

Add a backup solution if unit cell is not defined/usable

Open MBartkowiakSTFC opened this issue 1 year ago • 4 comments

Description of work In case no unit cell was defined for a system (or the cell is set to 0), calculates the maximum r in DistHistCutoffConfgurator from the span of atom coordinates.

This does not mean that the analysis results will be correct. For now, this PR will stop the GUI from crashing.

closes #516

Fixes

  1. Added a backup calculation of maximum r for DistHistCutoffConfigurator.
  2. Added a preview of the first unit cell in trajectory view.

To test Load a trajectory without a unit cell (Gaussian, xtb, Dmol3, ...?). Check that the GUI does not crash when Pair Distribution Function or Static Structure Factor are selected.

MBartkowiakSTFC avatar Jul 23 '24 15:07 MBartkowiakSTFC

This PR has now reached the stage where:

  1. The GUI does not crash if the trajectory has no unit cell,
  2. The Molecular Viewer / Trajectory Info shows a preview of the first unit cell dimensions,
  3. Pair Distribution Function can be run when no unit cell is defined. It then outputs warnings every time a fake unit cell is constructed by DistanceHistogram,
  4. Static Structure Factor and Density will both fail if no unit cell is defined or the cell volume is 0.
  5. Plotter will not crash if a dataset consisting of NaN or Inf is loaded for plotting.

A separate problem has been discovered: in trajectories containing only a single atom of a specific element (and any number of other atoms), the total PDF will be NaN. Added as issue #520

MBartkowiakSTFC avatar Jul 24 '24 11:07 MBartkowiakSTFC

Following your recommendation, different analysis types now fail if unit cell is not defined, but they recommend TrajectoryEditor as a backup solution, which allows adding a unit cell definition to a trajectory that did not have one.

TrajectoryEditor has been added, together with a new UnitCellConfigurator and Widget. It has its own unit tests, confirming that it can remove frames, remove atoms, change chemical elements and add a fixed unit cell to a trajectory.

MBartkowiakSTFC avatar Jul 26 '24 13:07 MBartkowiakSTFC

I've loaded a trajectory without a unitcell and used the trajectory editor to add one. This seems to work fine but when I run the static structure factor or pair distribution function it doesn't seem to produce any results.

image

Also, the job crashes when using the atom selection with the trajectory editor. The following error logs are produced.

2024-08-07 10:17:50,500 - CRITICAL - process[28196] - IJob 418 - Job failed with traceback: Traceback (most recent call last):
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\Framework\Jobs\IJob.py", line 410, in run
IJob._runner[mode](self)
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\Framework\Jobs\IJob.py", line 253, in _run_singlecore
idx, result = self.run_step(index)
^^^^^^^^^^^^^^^^^^^^
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\Framework\Jobs\TrajectoryEditor.py", line 156, in run_step
com_conf = PeriodicRealConfiguration(
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\MolecularDynamics\Configuration.py", line 215, in __init__
super(_PeriodicConfiguration, self).__init__(
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\MolecularDynamics\Configuration.py", line 59, in __init__
self[k] = np.array(v, dtype=float)
~~~~^^^
File "C:\Users\xcb63893\PycharmProjects\MDANSE\MDANSE\Src\MDANSE\MolecularDynamics\Configuration.py", line 111, in __setitem__
raise ValueError(
ValueError: Invalid item dimensions for velocities; a shape of (297, 3) was expected but data with shape of (300, 3) was provided.

ChiCheng45 avatar Aug 07 '24 08:08 ChiCheng45

Is it possible that the first problem was in fact not related to the cell definition, but instead this: https://github.com/ISISNeutronMuon/MDANSE/issues/520 ? As for the way the velocities are handled, thanks for spotting it. I will correct it.

MBartkowiakSTFC avatar Aug 19 '24 15:08 MBartkowiakSTFC

With the current version, I was able to run TrajectoryEditor with atom selection, and run Static Structure Factor calculations on the resulting trajectory afterwards. If the trajectory you tried to analyse was one of those with a single atom of one element, please run the TrajectoryEditor on it again, this time setting a unit cell and removing that single atom. If you are able to run the Static Structure Factor on the resulting trajectory, then the other problem you faced was most likely the bug I mentioned above, and not a problem with the TrajectoryEditor itself.

MBartkowiakSTFC avatar Aug 23 '24 13:08 MBartkowiakSTFC

Distinct van Hove function calculation now has a test for the presence of a valid unit cell. The job will fail if the unit cell is missing or set to zero size.

MBartkowiakSTFC avatar Sep 18 '24 13:09 MBartkowiakSTFC

Looks good. I've tested it out and it all works as expected. I can approve and merge once the conflicts have been fixed.

ChiCheng45 avatar Sep 19 '24 09:09 ChiCheng45

The conflicts have been resolved. I also added the check for the valid unit cell to the self van Hove function, and another check to the DistHistCutoffConfigurator. I think it should be ready now.

MBartkowiakSTFC avatar Sep 19 '24 10:09 MBartkowiakSTFC

The self van Hove function had the maximum distance cutoff switched off so that it doesn't have a maximum. It's similar to the situation for diffusion and mean square displacement. Can you revert that change?

ChiCheng45 avatar Sep 19 '24 12:09 ChiCheng45

I removed the unit cell check from the self van Hove function.

MBartkowiakSTFC avatar Sep 19 '24 12:09 MBartkowiakSTFC