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

Introducing state #11

Closed
wants to merge 6 commits into from
Closed

Introducing state #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba77e76
Added AbstractCalculator abstract type.
CedricTravelletti Dec 4, 2023
568b0df
Added state handling fallbacks.
CedricTravelletti Mar 9, 2024
a9be1c4
Removed AbstractCalculator type.
CedricTravelletti Mar 14, 2024
f3f960c
fix
CedricTravelletti Mar 14, 2024
7c672e4
Export calculator_state and update_state.
CedricTravelletti Mar 19, 2024
15876d6
Removed DummyState. Now use Missing.
CedricTravelletti Apr 4, 2024
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
2 changes: 2 additions & 0 deletions src/AtomsCalculators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module AtomsCalculators

export AbstractCalculator


using AtomsBase
using StaticArrays
Expand Down
6 changes: 6 additions & 0 deletions src/interface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@

abstract type AbstractCalculator end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not add AbstractCalculator type. It is not needed and it's presence will cause a lot of problems.


struct DummyState end
Copy link
Collaborator

Choose a reason for hiding this comment

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

DummyState is not needed. We just use Missing to do the same

calculator_state(::AbstractCalculator) = DummyState()
update_state(::Any, ::DummyState, ::Any, ::Any) = DummyState()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use

calculator_internal_state(::Any) = missing

update_calculator_state(
    update_algorithm::Any,
    initial_internal_state::Missing,
    initial_state::Any,
    new_state::Any
) = missing    # return new internal state

I would also add documentation on why to do this.

Also, update_algorithm is potentially very problematic. This stems from where are you going to define it. If it is defined for individual packages (like DFTK) then it should not be in the interface, or it will cause problems.

You will potentially call the command with different calculator types (e.g. when comparing different methods) and this causes errors, if algorithm in not defined for both of them. You could ignore algorithm all together to solve this. But this then goes on to the question, why it is there in the first place?


struct Energy end
struct Forces end
struct Virial end
Expand Down
Loading