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

Added check_simulation_cell in utils #117

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

JosePizarro3
Copy link
Collaborator

@JFRudzinski @ladinesa I added this utils function to check if a simulation cell is the same. "Same" here means that I check the positions in the lattice + the chemical symbol to be the same in each position. I am sure this can be extended, but for now, I need this when resolving workflows.

@ladinesa would you mind taking a quick look? I am pretty sure the check of the positions can be optimized, but I was lazy + ChatGPT is suggesting the wrong stuff (basically using numpy functions with some args that cannot be passed...).

Added equal cell tolerance

Added TODO to check ase in the future
@JosePizarro3
Copy link
Collaborator Author

Hi, so I checked ASE and the only option I saw is to directly check the positions and chemical symbols by using the functions get_positions and get_chemical_symbols. But this would only work for AtomicCell; our Cell is a more abstract object than Atoms, but it is more correct in our situation imo (we are considering the lattice to be an abstraction to the material, which is what is formally correct).

Do you mind doing a final swap and telling me if something is off?

@coveralls
Copy link

coveralls commented Sep 17, 2024

Pull Request Test Coverage Report for Build 10917079315

Details

  • 41 of 43 (95.35%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 80.308%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/model_system.py 38 39 97.44%
src/nomad_simulations/schema_packages/utils/utils.py 2 3 66.67%
Totals Coverage Status
Change from base Build 10811203881: 0.3%
Covered Lines: 2031
Relevant Lines: 2529

💛 - Coveralls

@ladinesa
Copy link
Collaborator

ladinesa commented Sep 17, 2024

Hi, so I checked ASE and the only option I saw is to directly check the positions and chemical symbols by using the functions get_positions and get_chemical_symbols. But this would only work for AtomicCell; our Cell is a more abstract object than Atoms, but it is more correct in our situation imo (we are considering the lattice to be an abstraction to the material, which is what is formally correct).

Do you mind doing a final swap and telling me if something is off?

Looks good but I thought you would implement __eq__ for Cell? Also I would really use typing checking instead of checking for m_def.name, use local import to rid of cyclic de.

@JosePizarro3
Copy link
Collaborator Author

Looks good but I thought you would implement __eq__ for Cell? Also I would really use typing checking instead of checking for m_def.name, use local import to rid of cyclic de.

Ah true, sorry I didn't understand this part before. I can implement that and it makes all the sense.

@JosePizarro3
Copy link
Collaborator Author

Much better now. What do you think?

@ladinesa
Copy link
Collaborator

Much better now. What do you think?

indeed just a couple more suggestions.

@JosePizarro3
Copy link
Collaborator Author

Done! Many thanks for the help 🙂

@JosePizarro3 JosePizarro3 merged commit 96f4a91 into develop Sep 18, 2024
7 checks passed
@JosePizarro3 JosePizarro3 deleted the utils_check_simulation_cell branch September 18, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants