Skip to content
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

Enable import of local XYZ files #108

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

mrakitin
Copy link

@mrakitin mrakitin commented Mar 1, 2023

Summary

This PR adds a feature to import local XYZ files. Partially based on the implementation in #26 with some subsequent cleanups and reusing objects from existing libraries.

Closes #26.

Example

An example input was taken from https://www.gloriabazargan.com/blog/visualizing-molecules:

image

One needs to remove the first two lines to make it work with the current version of the parser:

17       <-- remove this line
Pentane  <-- remove this line
C      -0.06119     -0.14438     -0.09006
H      -0.06046     -0.76500      0.81047
H       0.04110     -0.82551     -0.94388
C       1.13633      0.80196     -0.06785
H       1.04730      1.50136      0.77446
H       1.16482      1.40068     -0.98213
C       2.43464      0.02076      0.06562
H       2.42913     -0.60381      0.96237
H       3.28623      0.69990      0.13111
H       2.58851     -0.63256     -0.80327
C      -1.39343      0.59934     -0.18301
H      -1.35021      1.39778     -0.92849
H      -1.60573      1.07576      0.78352
C      -2.52277     -0.35635     -0.53888
H      -3.48791      0.04616     -0.21351
H      -2.38991     -1.33607     -0.06587
H      -2.57051     -0.50852     -1.61872

Screenshot

image

newMaterial.cleanOnCopy();

Made.tools.material.getBasisConfigTranslatedToCenter(newMaterial);
this.props.onSubmit([newMaterial]);
Copy link
Member

Choose a reason for hiding this comment

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

This function does the job, but it's a bit too long. Here's what we can do:

  1. Isolate the Math-related portions to Made
  2. Create one more default material config in Made to have a simple cubic lattice and import here
  3. Keep only 5-10 lines here with clear naming - e.g. determineMaxInteratomicDistance(material) etc.

// Line 150:17: Expected 'this' to be used by class method 'renderFooter' class-methods-use-this
// renderFooter() {
// return null;
// }
Copy link
Member

Choose a reason for hiding this comment

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

We can remove these, I guess, no?

material: PropTypes.object,
};

export default DefaultImportModalDialog;
Copy link
Member

Choose a reason for hiding this comment

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

We should consider a test for this functionality, similar to https://github.com/Exabyte-io/materials-designer/tree/dev/tests/cucumber/features/menu and including some of the fixtures you provided earlier in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants