-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Define specs #173
Conversation
Inspired by the work of @liamhuber in pyiron/pyiron_contrib#743 |
There was a problem hiding this 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". |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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.
## 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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
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. |
There was a problem hiding this comment.
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.
@samwaseda following our discussion today, I created a first set of specs for the atomistics package.