Do we actually need the `AbstractSystem` type?

JuliaMolSim / AtomsBase.jl

A Julian abstract interface for atomic structures.

https://juliamolsim.github.io/AtomsBase.jl/

MIT License

84 stars 18 forks source link

Do we actually need the `AbstractSystem` type? #125

Open rkurchin opened 3 weeks ago

rkurchin commented 3 weeks ago

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

jgreener64 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.