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

Unpin ASE, fix test #627

Merged
merged 3 commits into from
Feb 6, 2025
Merged

Unpin ASE, fix test #627

merged 3 commits into from
Feb 6, 2025

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Jul 24, 2024

fixes #612

@danielhollas danielhollas added the dependencies Pull requests that update a dependency file label Jul 24, 2024
@danielhollas danielhollas force-pushed the unpin-ase branch 2 times, most recently from ca18876 to 52de8b8 Compare July 25, 2024 16:09
@danielhollas danielhollas added this to the v0.2.3 milestone Sep 24, 2024
@yakutovicha yakutovicha removed this from the v2.3.0 milestone Feb 5, 2025
@danielhollas
Copy link
Contributor Author

Looks like the failing test is just that new ASE version now prints more digits in the cif file (xyz files are unchanged).

Here's the cif file

data_image0
_chemical_formula_structural       Si2
_chemical_formula_sum              "Si2"
_cell_length_a       3.84737
_cell_length_b       3.847369863377448
_cell_length_c       3.847369616935836
_cell_angle_alpha    59.99999709797012
_cell_angle_beta     59.999996706409306
_cell_angle_gamma    59.99999882531759

_space_group_name_H-M_alt    "P 1"
_space_group_IT_number       1

loop_
  _space_group_symop_operation_xyz
  'x, y, z'

loop_
  _atom_site_type_symbol
  _atom_site_label
  _atom_site_symmetry_multiplicity
  _atom_site_fract_x
  _atom_site_fract_y
  _atom_site_fract_z
  _atom_site_occupancy
  Si  Si1       1.0  0.0  0.0  0.0  1.0000
  Si  Si2       1.0  0.25000000000000006  0.25  0.25  1.0000

And here's the diff with the old version

diff test.cif test_new.cif 
5,9c5,9
< _cell_length_b       3.84737
< _cell_length_c       3.84737
< _cell_angle_alpha    60
< _cell_angle_beta     60
< _cell_angle_gamma    60
---
> _cell_length_b       3.847369863377448
> _cell_length_c       3.847369616935836
> _cell_angle_alpha    59.99999709797012
> _cell_angle_beta     59.999996706409306
> _cell_angle_gamma    59.99999882531759
26,27c26,27
<   Si  Si1       1.0  0.00000  0.00000  0.00000  1.0000
<   Si  Si2       1.0  0.25000  0.25000  0.25000  1.0000
---
>   Si  Si1       1.0  0.0  0.0  0.0  1.0000
>   Si  Si2       1.0  0.25000000000000006  0.25  0.25  1.0000

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.33%. Comparing base (0b1651b) to head (a91d758).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #627   +/-   ##
=======================================
  Coverage   83.33%   83.33%           
=======================================
  Files          17       17           
  Lines        3583     3583           
=======================================
  Hits         2986     2986           
  Misses        597      597           
Flag Coverage Δ
python-3.11 83.33% <ø> (ø)
python-3.9 83.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielhollas danielhollas marked this pull request as ready for review February 6, 2025 17:14
@danielhollas danielhollas changed the title Unpin ASE Unpin ASE, fix test Feb 6, 2025
@@ -105,6 +105,8 @@ def test_structure_data_viewer_storage(structure_data_object):
-1.6859999895095825, -1.6859999895095825, -0.6669999957084656, 1,
]
# fmt: on
# Avoid producing temporary files from povray in the repo
monkeypatch.chdir(tmp_path)
Copy link
Contributor Author

@danielhollas danielhollas Feb 6, 2025

Choose a reason for hiding this comment

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

This test produces Si2.png and __temp__.pov files so we change the current working directory to a tmp_path. Ultimately this should probably be fixed somewhere in the _render_structure code. I'll do that in a separate PR.

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

LGTM!

@danielhollas danielhollas merged commit c712c45 into aiidalab:master Feb 6, 2025
11 checks passed
@danielhollas danielhollas deleted the unpin-ase branch February 6, 2025 18:38
danielhollas added a commit that referenced this pull request Feb 6, 2025
* Unpin ASE
* Fix cif test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ASE 3.23
3 participants