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.

Sign up for GitHub

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

Provide a shortcut to ase.build #66

Closed
wants to merge 1 commit into from
Closed

Provide a shortcut to ase.build #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ The `structuretoolkit` is currently under development.

```python
import structuretoolkit as stk
from ase.build import bulk

structure = bulk("Al", cubic=True)
structure = stk.build.ase.bulk("Al", cubic=True)
stk.analyse.get_adaptive_cna_descriptors(structure)
stk.plot3d(structure)
```
Expand All @@ -43,6 +42,7 @@ stk.plot3d(structure)
* `stk.analyse.get_strain()`

### Build
* `stk.build.ase` (Merely a shortcut to the `ase.build` module)
* `stk.build.get_grainboundary_info()`
* `stk.build.grainboundary()`
* `stk.build.high_index_surface()`
Expand Down
3 changes: 2 additions & 1 deletion structuretoolkit/build/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from ase import build as ase # Just a shortcut structuretoolkit.build.ase == ase.build
from structuretoolkit.build.aimsgb import (
grainboundary,
get_grainboundary_info
Expand All @@ -8,4 +9,4 @@
from structuretoolkit.build.surface import (
high_index_surface,
get_high_index_surface_info
)
)
14 changes: 14 additions & 0 deletions tests/test_ase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# coding: utf-8
# Copyright (c) Max-Planck-Institut für Eisenforschung GmbH - Computational Materials Design (CM) Department
# Distributed under the terms of "New BSD License", see the LICENSE file.

import unittest

from ase import build

from structuretoolkit.build import ase


class TestAse(unittest.TestCase):
def test_build_shortcut(self):
self.assertIs(ase, build, msg="Our link should be a simple shortcut.")
Copy link
Member

Choose a reason for hiding this comment

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

@liamhuber Can you extend the test a bit, to test the typical functionality of the ase.build module? My fear is that ASE might change their functionality at some point and we only notice it by users complaining. For the same reason I added tests for the most commonly used functions from pyscal in https://github.com/pyiron/structuretoolkit/blob/main/tests/test_pyscal.py

Copy link
Member Author

Choose a reason for hiding this comment

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

@liamhuber Can you extend the test a bit, to test the typical functionality of the ase.build module?

I don't think we should do this. Right now the promise in the documentation, the implementation in the code, and the test are all in full alignment: we are simply providing a shortcut to another module, structuretoolkit.build.ase is ase.build.

My fear is that ASE might change their functionality at some point and we only notice it by users complaining.

Before this change, users had to keep their ase.build calls up-to-date with ase; after this changes users need to keep their ase.build calls (via stk.build.ase) up-to-date with ase. No changes here. If they complain all we need to do is let them know ase has updated and the rest is on them; if they want to downgrade their version of ase to keep their existing code working there's nothing that stops them -- all this does is link to the ase library.

For the same reason I added tests for the most commonly used functions from pyscal in https://github.com/pyiron/structuretoolkit/blob/main/tests/test_pyscal.py

First, I see this as a fundamentally different case. These tests check the behaviour of the public-facing methods stk.common.ase_to_pyscal, stk.analyse.get_steinhardt_parameters, stk.analyse.get_centro_symmetry_descriptors, stk.analyse.get_adaptive_cna_descriptors, get_diamond_structure_descriptors and stk.analyse.get_voronoi_volumes (btw I see now stk.analyse.find_solids is missing!), all of which are functions with actual bodies that we implement in str.common and stk.analyse.pyscal. Therefore it's right and good that we test them.
In contrast, stk.build.ase has zero content from our end. Any tests would just be some sort of duplication of ase tests and are unnecessary. We have no interface or promises to maintain other than that we are giving access to ase, which is what's tested.

Second, I think it would offer very little benefit. If we are testing just "common" cases (e.g. ase.build.bulk), then we're going to see tests fail elsewhere anyhow, since we use ase.build everywhere anyway! So testing standard calls would just be dead weight.

So, I am very much against expanding the tests beyond this -- BUT, I think it's perfectly fair to ask whether we want to provide a shortcut at all! The "pro" is that we can have a single point of import for a bunch of examples/tests/etc., which is great for lazy people like me. The "cons" are that we may come across as "stealing" ase functionality (I think this is not too bad since we explicitly call it build.ase) and your fear that users may mistakenly think we're the ones responsible for ase.build staying backwards-compatible.

I wanted to get your eyes on this in case you felt we should reject the idea in its entirety. I'm in favour of merging it (obviously), but I would accept your veto with zero hurt feelings.

Loading