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

Use Dataclass for ASE and QE #131

Merged
merged 15 commits into from
Dec 13, 2023
Merged

Use Dataclass for ASE and QE #131

merged 15 commits into from
Dec 13, 2023

Conversation

jan-janssen
Copy link
Member

No description provided.

@jan-janssen jan-janssen added the format_black Launch the pyiron/actions black formatting routine label Dec 13, 2023
@samwaseda
Copy link
Member

From my point of view, it makes more sense to have pre-defined fields in AtomisticsOutput, such as energy, forces etc., since we have already seen in pyiron_atomistics that by not defining fields, we could easily end up having different tags for the same values (cf. SPHInX & VASP)

@jan-janssen
Copy link
Member Author

From my point of view, it makes more sense to have pre-defined fields in AtomisticsOutput, such as energy, forces etc., since we have already seen in pyiron_atomistics that by not defining fields, we could easily end up having different tags for the same values (cf. SPHInX & VASP)

Done

Comment on lines 65 to 70
if "calc_energy" in tasks:
quantities.append("energy")
if "calc_forces" in tasks:
quantities.append("forces")
if "calc_stress" in tasks:
quantities.append("stress")
Copy link
Member

Choose a reason for hiding this comment

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

Here I see the same problem as the one we discussed here and here. To the very least, these lines appear twice inside atomistics, so they have to be somehow refactored, but more fundamentally, I see a clear connection between tags, tasks (i.e. calc_XYZ) and interactive getters. Interactive getters and the tags are now fully conceptualised with the data class, and I think the same should happen for the tasks as well.

Copy link
Member

Choose a reason for hiding this comment

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

110%, although fair to break that step into a different PR

@liamhuber
Copy link
Member

It's not mission critical because one can always refactor: rename later, but it might be wise to resolve #129 before/while propagating these classes to the other calculators

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.

There's still no concept for the part that I mentioned, but it's now refactored, so I'm ok with the current state

@jan-janssen
Copy link
Member Author

There's still no concept for the part that I mentioned, but it's now refactored, so I'm ok with the current state

I agree I played around with it a bit and was not able to solve it, so I would prefer to move forward with the current pull request and then do the optimisation in a separate pull request.

@jan-janssen jan-janssen merged commit 80a1e9b into main Dec 13, 2023
17 checks passed
@jan-janssen jan-janssen deleted the more_dataclasses branch December 13, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black Launch the pyiron/actions black formatting routine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants