Feat: xyz file parsing - nAtoms & nLines to check formatting. Created… by adewyer · Pull Request #52 · Exabyte-io/made

adewyer · 2021-12-30T20:10:16Z

… functions and tests

timurbazhirov · 2022-01-06T23:16:29Z

src/parsers/xyz.js


+export function atomsCount(xyzFile) {
+    const xyzArray = xyzFile.split(/\r?\n/);
+    return Integer.parseInt(xyzArray[0]);


Why do we even need this function, if all this does is just split on newlines? This is not specific to XYZ format, is it?

No docstring, explanation of the expected behavior

The first line of an xyz file contains the number of atoms. So we split the string by new lines and then return the first new line to get the number of atoms in the file.

timurbazhirov · 2022-01-06T23:16:48Z

src/parsers/xyz.js

+    const content = xyzFile.split(/\r?\n/);
+    if (!content[-1]) {
+        content.pop();
+    }


Not sure what's happening above - no explanation either.

…nd converting to poscar to made instead of web-app

adewyer · 2022-01-07T17:46:21Z

src/parsers/parsers.js

+ * @param {String} fileContent
+ * @returns {Number}
+ */
+export function getAtomsInFileByExtension(fileExtension, fileContent) {


getNumberOfAtomsByFileExtension

… for getting number of atoms in parsers

src/parsers/parsers.js

src/parsers/poscar.js

timurbazhirov · 2022-01-08T02:23:22Z

src/parsers/xyz.js

 import { ConstrainedBasis } from "../basis/constrained_basis";
 import { Lattice } from "../lattice/lattice";
+// eslint-disable-next-line import/no-cycle
+import { defaultMaterialConfig } from "../material";


I understand your comment above, but we should not be introducing circular dependencies with new code. It's usually a sign of not having a correct placement of the code

src/parsers/xyz.js

timurbazhirov · 2022-01-08T02:34:21Z

src/parsers/xyz.js

+    const xyzBasis = xyzArrayBasisOnly.join("\n");
+    xyzConfig.basis = toBasisConfig(xyzBasis);
+    xyzConfig.basis.units = "cartesian";
+    return poscar.toPoscar(xyzConfig);


The placement of this function seems to be bad at the moment, hence the circular dependency. We should move this functionality to resolve the problem - maybe to top-level of parsers?

adewyer · 2022-01-08T03:11:15Z

src/parsers/parsers.js

@@ -1,11 +1,47 @@
+// eslint-disable-next-line import/no-cycle
+import { defaultMaterialConfig } from "../material";


Moving the xyzToPoscar function from parsers/xyz.js to this file fixed the import xyz circular dependency, but created one for defaultMaterialConfig. Previously this circular dependency was noted inside parsers/xyz.js

adewyer · 2022-01-13T17:29:34Z

Not sure why CICD tests are failing. Everything passes locally

Basis
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
✔ should return true if basises are equal
✔ should return true when basis is compared to its clone
✔ should return jsonified basis
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
✔ should return true if cells are equal
✔ should return Na4Cl4 as unitCellFormula
✔ should return NaCl as formula
✔ should return elements
✔ should return unique elements
✔ should return elements count
✔ should set elements
✔ should return coordinates
✔ should set coordinates
✔ should return true if basis is in crystal units
✔ should convert crystal to cartesian
✔ should add a new atom
✔ should remove an atom
✔ should return number of atoms
✔ should return the string
✔ should return standard representation
✔ should return minimum lattice size for a molecule
Basis.constructor: units are not provided => set to crystal
✔ should return minimum lattice size for an atom
✔ should return max distance
✔ should return center of coordinates
✔ should return the updated basis coordinates

Cell
✔ should return scaled cell

Primitive Cell
✔ should return primitive lattice

AtomicConstraints
✔ should return constraints as string
✔ should return constraints as string with given map function
✔ should return [true, true, true] as constraints for the line without any constraints

Lattice
✔ should return lattice cell volume
✔ should return lattice from vectors
✔ should return lattice type
✔ should return lattice hash string

Lattice Bravais
✔ should return lattice bravais from vectors
✔ should return a list of editable keys

Lattice Reciprocal
✔ should extract kpoint path
✔ should return cartesian coordinates of a point
✔ should return reciprocal vectors
✔ should extract symmetry points

Lattice Vectors
✔ should return lattice from bravais

Parsers:Espresso
Basis.constructor: units are not provided => set to crystal
✔ should return textual representation of a material according to QE pw.x input format
(node:19113) DeprecationWarning: sprintf() will be removed in the next major release, use the sprintf-js package instead.

Parsers:XYZ
✔ should return the xyz file content in poscar file format

Parsers.POSCAR
✔ should return a valid poscar
✔ should return poscar elements line according to id in basis, duplicate entries separate
✔ should return the number of atoms for a molecule in a poscar file

Parsers:XYZ
✔ should extract basis from XYZ text
✔ should return the number of atoms for a molecule in an xyz file

Parsers:CombinatorialBasis
Basis.constructor: units are not provided => set to crystal
✔ toBasisConfig
Basis.constructor: units are not provided => set to crystal
✔ Regular XYZ
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
✔ Permutation XYZ
✔ Combination XYZ
✔ Mixing combination and permutation should fail

Tools:Basis
Basis.constructor: units are not provided => set to crystal
✔ should return a repeated basis
✔ should return interpolated basises

Tools:Supercell
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
✔ should generate supercell

Tools:Surface
Basis.constructor: units are not provided => set to crystal
Basis.constructor: units are not provided => set to crystal
✔ should return slab

56 passing (105ms)

adewyer · 2022-01-13T17:37:56Z

.github/workflows/cicd.yml

@@ -18,6 +18,8 @@ jobs:
    steps:


Fixes from @tjduigna to fix CICD test failures caused by git lfs not pulling test files.

… feature/SOF-5388

timurbazhirov · 2022-02-12T01:44:21Z

src/default_material.js

+    },
+    lattice: {
+        // Primitive cell for Diamond FCC Silicon at ambient conditions
+        type: LATTICE_TYPE.FCC,


I suspect the fact that we are using LATTICE_TYPE here and in the Lattice, and then import Lattice inside Material could lead to circular dependency which could lead to the problem with tests for https://github.com/Exabyte-io/exabyte-stack/pull/235/files#r805085142

okay i will look into this.

We saw these same errors prior to me moving default materials out of material.

Previously we imported both LATTICE_TYPE & LATTICE into material.js. Wouldn't that lead to the same circular error?

…to poscar

timurbazhirov · 2022-02-17T03:19:26Z

src/default_material.js

+ * @returns {Object}
+ */
+export function getDefaultMaterialConfig() {
+    return defaultMaterialConfig;


return {...defaultMaterialConfig}

Note: utilize this function instead of defaultMaterialConfig in code.

Feat: xyz file parsing - nAtoms & nLines to check formatting. Created…

ac672c8

… functions and tests

timurbazhirov reviewed Jan 6, 2022

View reviewed changes

Feat: updates to move all functions for dealing w/ parsing xyz file a…

c06358a

…nd converting to poscar to made instead of web-app

adewyer commented Jan 7, 2022

View reviewed changes

simplified atomsCount function for xyz file and updated function name…

e5e38b6

… for getting number of atoms in parsers

adewyer commented Jan 7, 2022

View reviewed changes

src/parsers/parsers.js Outdated Show resolved Hide resolved

adewyer added 2 commits January 7, 2022 10:07

removed unused function from parsers

aef4e7b

updated tests for new function naming

65c6528

timurbazhirov reviewed Jan 8, 2022

View reviewed changes

src/parsers/poscar.js Outdated Show resolved Hide resolved

timurbazhirov reviewed Jan 8, 2022

View reviewed changes

src/parsers/xyz.js Outdated Show resolved Hide resolved

timurbazhirov reviewed Jan 8, 2022

View reviewed changes

moved functions and updated naming based on PR comments

6ef320c

adewyer commented Jan 8, 2022

View reviewed changes

moved defaultMaterialConfig to it's own file to avoid circular imports

78572e9

adewyer force-pushed the feature/SOF-5388 branch from a7520d7 to 78572e9 Compare January 13, 2022 17:26

feat: git lfs pull

df31214

adewyer commented Jan 13, 2022

View reviewed changes

adewyer added 2 commits January 13, 2022 12:09

remove eslint-disable-next-line import/no-cycle

bce5e1b

Merge branch 'feature/SOF-5388' of github.com:Exabyte-io/made-js into…

2877ebc

… feature/SOF-5388

timurbazhirov reviewed Feb 12, 2022

View reviewed changes

adewyer added 3 commits February 11, 2022 18:29

updated functions for converting between formats

d859afc

updated how we copy the default material in converting other formats …

8750dae

…to poscar

updates for converting xyz --> poscar without altering default material

431a269

timurbazhirov reviewed Feb 17, 2022

View reviewed changes

timurbazhirov added the come-back-later label Nov 22, 2023

		@@ -1,11 +1,47 @@
		// eslint-disable-next-line import/no-cycle
		import { defaultMaterialConfig } from "../material";

Comments

Conversation

adewyer commented Dec 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adewyer Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adewyer commented Jan 13, 2022

Uh oh!

adewyer Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adewyer Jan 8, 2022 •

edited

Loading

adewyer Jan 13, 2022 •

edited

Loading