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

Do we actually need the AbstractSystem type? #125

Open
rkurchin opened this issue Oct 23, 2024 · 3 comments
Open

Do we actually need the AbstractSystem type? #125

rkurchin opened this issue Oct 23, 2024 · 3 comments
Labels
question Further information is requested

Comments

@rkurchin
Copy link
Collaborator

I think there may already be reasonable consensus that the answer is yes, but I wanted some of these discussions from the workshop to be archived and for other folks to have a chance to chime in if they want.

Arguments for getting rid of it:

  • people could subtype something else, e.g. AbstractArray
  • interface functions could still work without the explicit type...sort of (see below)

Arguments against:

  • there are some explicit examples where dispatching on this is needed, e.g. from @mfherbst here and here and I think someone yesterday mentioned another case, might have been @tjjarvinen so he might be able to give others
  • as @swyant mentioned, keeping it also improves readability of code so that someone reader can understand generally what's expected
@rkurchin rkurchin added the question Further information is requested label Oct 23, 2024
@jgreener64
Copy link
Collaborator

I don't feel strongly enough to push hard for its removal, but I think if it didn't already exist we probably wouldn't add it.

@mfherbst
Copy link
Member

mfherbst commented Oct 23, 2024

I don't think we can do without it. I agree a system argument should not be type-annotated unless needed, but the two examples above from DFTK are cases where removing the abstract type can lead to bugs, which are very hard to understand for new users to Julia, so it would substantially increase the entrance barrier to DFTK.

Also personally I always forget in AtomsCalculator: Is it potential_energy(system, calculator) or potential_energy(calculator, system). Having at least one argument annotated with a type helps to catch such bugs in a non-trivial workflow script. Type-annotating calculator is out because there we definitely want full flexibility in my opinion. I think on the system the chances are higher we agree on some consensus.

@cortner
Copy link
Member

cortner commented Oct 23, 2024

I also think at least right now there is not much point in removing this abstract type, especially because it allows us to implement generics. What we could discuss is whether code such as DFTK, Molly etc should allow systems as inputs that are not derived from AbstractSystem but implement the interface. That part could be potentially useful but I also don't have an immediate urgent use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants