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

Define specs #173

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Define specs #173

wants to merge 1 commit into from

Conversation

jan-janssen
Copy link
Member

@samwaseda following our discussion today, I created a first set of specs for the atomistics package.

@jan-janssen
Copy link
Member Author

Inspired by the work of @liamhuber in pyiron/pyiron_contrib#743

Copy link
Member

@samwaseda samwaseda left a comment

Choose a reason for hiding this comment

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

Sorry I was convinced that I had left my review last week. It's a nice basis and I think we can have a good discussion. Now I rewrote my comments, but they are mostly in order to initiate a discussion so you don't need to give me replies immediately.

In general, I have a bit of the problem, that it's still not really clear what are the steps for someone coming from outside with their code. Since there's already a good output abstraction, it definitely has to be mentioned here. Other than that, the same stuff is missing for the input, and for different modes (static, interactive), the concept is still missing. It's not a problem of this document, but for a spec to be useful for atomistics, these points have to be clarified.

the resulting internal structure of the `atomistics` package.

## Vision
The `atomistics` package provides "interfaces for atomistic simulation codes and workflows".
Copy link
Member

Choose a reason for hiding this comment

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

This part fails to define what atomistics actually means. I think the easiest would be to base it on ASE Atoms. Then it would be something like "Atomistic simulation codes are those, which extract physical information of atomistic structures, given either by ASE Atoms or a set of properties that can be translated to ASE Atoms".

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't have to be this wording, but I think we need something that clearly defines the borders

Comment on lines +10 to +26
To calculate a physical property with an atomistic simulation code, there are typically two ways:

* **internal implementation**: The simulation code already provides the internal functionality to calculate the physical
property. Most atomistic simulation codes can calculate, potential energies and forces for a given atomistic
structure. Some simulation codes already provide internal functionality to calculate more complex physical properties,
like the phonon density of states or energy barriers using the nudged elastic band method.
* **external tools**: The second approach is to use a workflow, which combines a series of individual calculation of
atomistic simulation codes to calculate physical properties. These workflows can be simple shell scripts for example
for the calculation of energy volume curves or more complex physical properties, like the phonon density introduced
above.

The challenge for the `atomistics` package is balancing both approaches. The first is commonly computationally more
efficient, while the second allows to directly compare different simulation codes and simulation methods. The
`atomistics` packages provides an abstraction to balance between both approaches. By default, it uses the internal
implementation when available and falls back to a universal python based implementation when necessary. With this
approach the `atomistics` package allows users to calculate a wide-range of physical properties with a wide-range of
simulation codes.
Copy link
Member

Choose a reason for hiding this comment

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

I find it very nice that it addresses a main problem here. However, it doesn't really address how we approach it. For example, based on this description I would not know where to go if I have my own atomistic code. I think it's important that we refer to the output classes at least. In addition to this, we would restructure the input classes to have a similar modularity, and the interactive mode has to be conceptually somehow included. This being said, it's not a problem of this document, but it's more a fundamental problem of how this module is structured, so I'm also ok with not mentioning it here right away.

Comment on lines +55 to +66
## Structure
The `atomistics` package is structured into the three following modules.

### `atomistics/calculators`
Interfaces for the internal implementations of the individual simulation codes to calculate a specific physical property.

### `atomistics/shared`
Classes, functions and modules which are used for both the internal interfaces and the external tools.

### `atomistics/workflows`
Interfaces for external tools to calculate a specific physical property by combining physical properties calculated from
individual simulation codes.
Copy link
Member

Choose a reason for hiding this comment

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

This is not a spec, but only says what kind of folders exist. There's an important difference to a concept here, because these folders are currently defined merely for the technical aspect, but it doesn't have a conceptual structure. I see the biggest problem in shared, because anything that is shared by two codes simply go there, even though except for the output classes there's no clear abstraction. So this part can maybe go to README, but it's not part of specs.

for scientists compared to working with classes. Classes are only used when transferring data between function calls.

### Dictionary based Interfaces
Each function accessible to the users should return a dictionary to name the output of the function.
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Comment on lines +33 to +34
The use of functions is preferred over the use of classes as applying and developing functions is typically easier to
for scientists compared to working with classes. Classes are only used when transferring data between function calls.
Copy link
Member

Choose a reason for hiding this comment

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

That's very nice, but the question is also how the functions are bundled. I currently don't see a clear structure for this.

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.

2 participants