-
Notifications
You must be signed in to change notification settings - Fork 16
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 really need separate AbstractParticle
and AbstractElement
types?
#7
Comments
I think removing this extra type is possibly a good idea, though if we do, I think But I'm curious for @mfherbst's thoughts on this, as I feel like he had a reason originally for there to be a type for this...does it have to do with things that aren't spherically symmetrical needing to know about orientation or something like that perhaps? |
I am not sure about returning subsystem for single index. For example, base julia arrays act differently if you use a single index or a range. In that regards, this is a really good question what kind of behaviour we want when we index with range: do we want to copy data and create a totally new system, or do we want to create some kind of view into existing system? |
To summarize some recent discussions we've had on this (in case this issue stays open when we start publicizing the repo for broader feedback): The (mostly-consensus) understanding is that Pragmatically, the utility in separating these is that in the context of running a simulation, @Gregstrq still has some concerns around how these types of parameters could get "mixed together" in inappropriate ways. I'm working on a sample implementation for an SoA version to complement the existing AoS one, which will hopefully help clarify (and/or reify) some of these issues. |
It sounds to me rather that you want to have the properties of a particle divided into constants and variables. Isn't that overthinking it a bit at this level of abstraction? One user might want the mass to be a constant, the next a variable? Maybe there could be an implementation of |
To my mind, the main use is the thing I said towards the end of the above comment, that you can use the information in an Conceptually, this splitting into a sort of "generic identity" (in my mind this is usually a chemical element but of course it needn't always be) and a "specific instantiation" (e.g. an atom with a set of coordinates) makes sense, but it is entirely possible I'm too stuck in the DFT reference frames and am missing something broader. Though I think this makes sense also for MD at least? |
I understood that, but this seems already like an advanced functionality. What I didnt understand why it needs to be in AtomsBase which is supposed to be lightweight? Also - why can't you look up the pseudopotentials if the species are stored in the AbstractParticle? |
I guess what it comes down to is whether
and then specify which parts of the interface are supported for which level? |
These are all fair points! I suppose out of those options, I probably like the adding a layer of abstraction option the best as it seems the most flexible. Keep the highest level the most generic, but still specify the interface for some more involved (and anticipated very common) use cases like what I describe above. The names of everything still TBD, of course... I guess one question I'd put back to you is: what specific use case do you envision where you wouldn't want some way to specify the types of particles like what this provides? |
Well, the idea was to use different subtypes of
Could you describe a use case with Empy Element? It seems that any kind of particle is bound to have some static physical properties which can be packed into a structure subtyping |
Sure - any context in which there is just one particle species. Often but not always these wil be tou models |
But more importantly it isn’t clear to me why this separation is needed /, useful |
Precisely because we can have single particle species or only a few species.
But each particle still has a concrete property: species it belongs to. |
But the interface seems to enforce it? |
And I'm not against having an interface for species at all. I'm just confused about the need to treat it separately from the concept of a Particle. Why not just have an property |
Because the concept of species is different in the different contexts. So, depending on the context, one has a different set of properties which describe what species is. Hence, depending on the context, we have different types (structs) that describe what species is. And since we have to deal with a multitude of types that correspond to the same concept, it is natural to introduce an abstract type that relates to this concept. |
That’s all fine and doesn’t contradict what I’m saying. I’m only against enforcing an extra type parameter in both an atom and a structure unless really needed. And I don’t see the need yet. |
We might have a static and dynamic systems consisting of the same species. Although the species are the same, the physical properties of the particles are different, because in the static case there is no velocity. |
What is the problem with that? The type can be automatically computed during construction, user does not have to worry about that. |
Still doesn’t prevent you from doing what I suggested |
First the user does have to worry because it is enforced on her by the interface. Secondly it makes the entire interface more complex than needed. It smells of over-engineering. |
Im really not against it if there is a strong software engineering argument for it but so far I see only downsides. |
I think it is useful from the interoperability perspective as having a type makes it easy to dispatch on, and this allows checks for various types of compatibility and/or interconversion between two systems expressed in different concrete implementations of the interface, but perhaps with the same Curious for @mfherbst's and @jgreener64's thoughts on all of this as well, though. |
Agree on I can see in principle maybe how this might help? At least that's the first point where I'm not sure anymore. Could you give a concrete example to explain what you are envisioning? How might such a conversion work without the authors of the two types manually implementing it? Because once you are back to that, then I again don't see what is gained. Regarding checks for compatibility ... I don't know. The default in Julia seems to me is, when it breaks, add the methods to make it work. In fact, I am not even 100% certain about the need for an I used to over-engineer my type hierarchies but eventually deleted almost all abstract types. Nowadays I always start with concrete types and only add abstract type layers when I start duplicating functionality + it is needed for dispatch. The moment you enforce some abstract types on some methods, you can no longer use them for arguments that aren't derived from those and since Julia forbids you to have more than one supertype that's a real problem. |
The interface for iterate is a nice example. |
I will confess upfront that I haven't thought this through in detail, but one hypothetical I keep coming back to for interoperability is imagining trying to implement AIMD in e.g. Molly by calling out to, e.g. DFTK as the force predictor, so I'll try to spin that out in a bit more detail now... Each simulator could (and very possibly would, since they'd be optimizing different things to maximize performance and might want to store data differently, etc.) use different concrete implementations of the interface. But let's suppose they both use the same And eventually, we can also plug this into plotting recipes that will know how to make nice visualizations with standardized colors/labels/etc. for different species again via dispatch on that parameter... In some sense, the chemical element case is a bit simplistic, since the elemental symbol is sufficient information. I think it gets more interesting in the case where the Maybe this helps? |
It helps in that I know what you envision but not in why the complex type hierarchy is needed. Where are you dispatching? |
To continue the example above, the DFT force computation function would need to take in a structure that would have the type parameter |
Still not clear why you need to dispatch on that. I get that it seems this way but it really doesn’t. All you need is get_chemical_species |
As others have mentioned, the difference isn't so much about static and dynamic information as information about a specific particle in the simulation (some I agree though that not all simulations make this distinction and we might not want to impose it. The benefit of imposing it as Rachel says is that people might start to reuse the same species types, i.e.
and then people could write functions that dispatch on that element type at the level of the system. The drawback is that some users might have to use empty containers. I am not sure about the multiple levels of abstraction idea, it replaces complexity with more complexity. |
I think we all agree that the vast majority of the simulations we will perform will want some for of species information and having the species "class" as type information is useful. This is not disputed. I am In the absence of multiple inheritance this really limits how we can use those. |
The way I see it, the interface is both a set of concepts and a set of operations on those concepts. Also, an abstract type hierarchy facilitates the communication about these concepts between the various developers using the same interface. If something is a subtype of an abstract type corresponding to a particular concept, then everyone knows that it implements that particular concept. It also helps in debugging interoperability issues between different libraries using the same interface. So, the real question here is whether some concept is an integral part of the interface. And it seems that everybody agrees that the concept of species is in fact an important part of the interface. |
again - nobody disputes that it is an important part of the interface. I'm only questioning whether it must be baked into quite as "in your face" as it is now. I appreciate that the second point about the need for an abstract type hierarchy will be more difficult for me to defend, but it is not as obvious as it might seem to you. Much of Base Julia is organized only by convention - again cf the iterator interface. When you look at how some of the most experienced Julia programmers approach package development, they typically don't bother with type hierarchies at all until it becomes necessary for organizing dispatch. Here, I haven't yet seen a concrete example that convinces me that we need it. So in summary: I agree it is critical to have a well-defined interface for what a particle is and what a species is. But it is not at all obvious to me that a type hierarchy is needed or indeed useful. |
Interesting discussion. I fully agree that it is always easier to add abstract types later rather than removing them. I have to say that (coming from the C++ world) I also only recently learned to appreciate the "design by convention" aspect. So if I get it right, what you propose @cortner is to define the interface like define which functions exist and what sort of conceptional entities they return, but not enforce an abstract type hierarchy for these entities. While my feeling is that this might be giving up quite a lot (too much?) of the structure of the problem, it might also make it easier to reach a concensus and push out a first version of the interface (less bike-sheding about names for example 😄). I'd generally be open for dropping the abstract types, e.g. on the element/species (btw we had species at some point in the past, right?). I'm not so sure about the system, however, because at that level in DFTK we would definitely need dispatching in the setup functions and I assume the same is true for other codes. Could that be a compromise worth pursuing? |
Hi Michael - thanks for considering this option. I think there are several possible layers:
I am actually not sure whether 2 or 3 is better. If I'd be designing my own code then I would always go for 3 since I can add abstract types later on at any time. But this is now a community package and once there are 5-8 implementations breaking things will not be good. So I do think that we need to come to a concensus. I'd also like to re-emphasize a previous point: I'm really not fundamentally against abstract type hierarchies, e.g. I quite like Java style interfaces. But (1) in Julia we don't get multiple inheritance; and (2) such a strict interface requires you to implement everything. Whereas in Julia we have the freedom to only implement the part of the interface that we need. It is really liberating. I think some thought experiments might help decide? ... coming up ... |
(1) Suppose e.g. I do some ML on atoms. Then an atom object (2) I keep asking myself why I need to put the species information into the type parameters? function get_pseudopotential(at::AbstractAtom)
sym = chemical symbol(at)
return get_this_from some_other_library(sym)
end (3) Worst-case scenario, if you really need to dispatch on the type of the species, you can do do_something(at::AbstractAtom) = do_something(at, get _species(at))
do_something(at::AbstractAtom, s::AbstractSpecies) = ...generic
do_something(at::AbstractAtom, s::ChemicalSpecies) = ...special How many cases can we envision where we need to do this? If many then that would be an indicator that we do need to consider putting the type information in (i.e. option 1). But I would be very surprised. I think it is much more likely that we will just write generic code and then if the concrete species doesn't implement some functionality it will break. (4) @mfherbst can you explain why you need this in the DFTK setup stage? |
Maybe an example from a different domain helps: I want to multiply |
@cortner, We have decided to go with the third option and drop all the abstract types except |
thanks for the update. I'm a bit unsure how this is different from 1? Anyhow ... I will see where it goes and then comment again. Time for me to shut up. |
To comment on your question Christoph:
In DFTK we have a certain flexibility to setup DFT calculations. The most high-level is |
I don't fully see the issue, but also I'm a lot less worried about the AbstractSystem than the AbstractAtom. Just to explain why I don't see it: why is it not enough to just define the interface for |
Essentially for compatibility with the interface as we support it now, where the lattice can be just a matrix of numbers or some python object (for ASE compatibility). Especially for the matrix of numbers case it's a bit hackish to define a |
To be clear though, I don't think there's anything stopping you from defining the interface functions on your own type but not actually subtyping And if it wouldn't work, then IMO that would be a problem and it's worth discussing. For example, could what we're after be achieved by some sort of trait instead of actually inserting something into the type hierarchy? I could imagine that other package developers might not always want to make their type a subtype of |
yes, that is precisely the point. It is not the end of the world, we just need to start converting then. But if we worked purely with an interface, then no need! |
I'm going to try gradually removing pieces and only ever commit working versions over at #23, so you can keep an eye out there. So far, I've pushed a version that removes Thanks @cortner for forcing us to think through all this really carefully; this will undoubtedly be better for it, wherever we land! |
I can see both sides but personally I would lean towards keeping Ironically the use of having something be a subtype of
I worry we lock ourselves out of this potential if we don't introduce |
I agree that’s another important point - to inherit behaviour. As I said I don’t have a strong view on AbstractSystem. That said - one can always “inherit” behaviour via a macro |
see my comments in the review. From here on I think I would need to test this. |
Right now we have two types describing our particles:
AbstractParticle
andAbstractElement
.AbstractElement
contains the physical information about the element, besides its coordinates and possibly velocities.At the same time
AbstractParticle
by definition is simply a container which storesAbstractElement
, vector of coordinates and possibly vector of velocities. SinceAbstractParticle
is simply a container, it does not add anything new in terms of functionality. Moreover, since it may or may not contain velocities, we need to create at least two concrete subtypes: one without velocities and one with them, which again leads to the redundancy in the interface (we need to implement he functions that deal withAbstractElement
and coordinates for both of these subtypes).In these regards, Rachel mentioned this dicsussion of the indexing in Xtals package.
So, I propose that we remove
AbstractParticle
type. We can renameAbstractElement
toAbstractParticle
although, but it would still correspond to element data without coordinates and velocities. Andgetindex
would then simply return a tuple (Element, coordinate) or (Element, coordinate, velocity).The text was updated successfully, but these errors were encountered: