simplify types #23

rkurchin · 2021-11-10T21:34:30Z

Starting to work on this. Will try to commit-by-commit remove more things (and only ever committing a working state) so if we decide we want to go only part of the way, it's easy to just revert. This present commit removes only the AbstractElement type, which, now that I've done it, seems like unambiguously a good idea and I don't think it was adding much of anything.

Signed-off-by: Rachel Kurchin <[email protected]>

…mmit, put them back Signed-off-by: Rachel Kurchin <[email protected]>

codecov-commenter · 2021-11-10T21:36:36Z

Codecov Report

Merging #23 (9a01ba9) into master (0007177) will increase coverage by 47.36%.
The diff coverage is 45.45%.

@@             Coverage Diff             @@
##           master      #23       +/-   ##
===========================================
+ Coverage    0.00%   47.36%   +47.36%     
===========================================
  Files           4        4               
  Lines          97       76       -21     
===========================================
+ Hits            0       36       +36     
+ Misses         97       40       -57

Impacted Files	Coverage Δ
src/implementation_soa.jl	`52.63% <40.00%> (+52.63%)`	⬆️
src/atoms.jl	`43.75% <43.75%> (ø)`
src/interface.jl	`35.71% <44.44%> (+35.71%)`	⬆️
src/implementation_aos.jl	`69.23% <66.66%> (+69.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0007177...9a01ba9. Read the comment docs.

Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin · 2021-11-10T22:20:43Z

Okay, there we have it! Personally, I feel quite good things up through fede982, but I'm on the fence about also removing AbstractSystem...I can't decide if what we gain in flexibility offsets what we would lose in being able to dispatch things with some guarantees about things like dimensionality, what sorts of particles make up the system, etc., the way we can with an AbstractSystem that would have those things as type parameters. Eager to discuss.

I've also made some notes to myself about other potential small tweaks, the applicability of some of which will depend on how many commits in this we keep, so I'll do them after we decide rather than cluttering up the history here.

This reverts commit 7a32c77.

Signed-off-by: Rachel Kurchin <[email protected]>

… file that somehow came back Signed-off-by: Rachel Kurchin <[email protected]>

…fix return types of other singular functions, rename ET parameter to S, add default size dispatch since firstindex default to a scalar Signed-off-by: Rachel Kurchin <[email protected]>

Signed-off-by: Rachel Kurchin <[email protected]>

jgreener64

Looks good to me, readme and tests are nice too.

Gregstrq

We need to add indexing versions of position and species for the concrete implementations.

README.md

Signed-off-by: Rachel Kurchin <[email protected]>

mfherbst

A few nits, but generally LGTM.

sandbox/aos_vs_soa.jl

src/atoms.jl

src/implementation_aos.jl

src/implementation_soa.jl

src/interface.jl

mfherbst · 2021-11-17T08:51:02Z

src/interface.jl

 Base.getindex(::AbstractSystem, ::Int) = error("Implement me")
-Base.size(::AbstractSystem) = error("Implement me")
 Base.length(::AbstractSystem) = error("Implement me")


I've only put the error("Implement me") here during development. I guess with the readme making this clear to be implemented we should drop these stubs completely (and let Julia throw an error for us). Thoughts?

I can see it both ways. On the one hand, it's redundant to throw an error "manually" here, but on the other, having all the required functions in the interface.jl file does make it a useful reference for someone who wants to sit down and implement the interface...

I would lean towards not putting the errors here, and perhaps adding a comment if we want the information to be in that file. See the first section of https://www.oxinabox.net/2020/04/19/Julia-Antipatterns.html for some discussion.

I agree with @jgreener64 (and oxinabox) on that point very much.

Is this intended to be implemented now during interface development? Then I think an error message is fine because it will flag that this is supposed to be implemented but isn't. But if it is an interface function that a user should implement, then I don't think the error message should be there.

Signed-off-by: Rachel Kurchin <[email protected]>

mfherbst

lgtm

cortner

looks good I think, just left a few fairly trivial comments.

cortner · 2021-11-17T17:31:08Z

src/implementation_aos.jl

-struct FlexibleSystem{D,ET<:AbstractElement,AT<:AbstractParticle{ET}} <: AbstractSystem{D,ET,AT}
-    box::SVector{D,<:SVector{D,<:Unitful.Length}}
+struct FlexibleSystem{D,S,L<:Unitful.Length,AT} <: AbstractSystem{D,S}
+    box::SVector{D,<:SVector{D,L}}


"morally" it seems to me that box and boundary_conditions should be tuples of ... rather than SVectors. But I don't have too strong a view on that.

cortner · 2021-11-17T17:33:30Z

src/implementation_aos.jl

 end
 # convenience constructor where we don't have to preconstruct all the static stuff...
 function FlexibleSystem(
-    box::Vector{Vector{L}},
+    box::Vector{<:AbstractVector{L}},


why type this? the box could be provided as a Vector, AbstractVector, Tuple, .... same with bc and particles below. This is a convenience constructor no? I would then just make it convenient to construct the thing and enforce such strict typing.

Yes, that would probably be better. I was having trouble figuring out how to write it in a general enough way...will file an issue for this too.

The way I did it in JuLIP was to have a conversion interface e.g.

_convert_positions(X::Vector) = X _convert_positions(X::AbstractVector) = collect(X) _convert_positions(X::AbstractMatrix) = # convert into vector of SVectors _convert_bc(bc::NTuple{3, Bool}) = bc _convert_bc(bc::AbstractVector) = tuple(Bool.(bc)...)

and so forth. So this means that ANY sensible input will be automatically converted into the format needed internally.

ok, will repost in the issue.

cortner · 2021-11-17T17:37:58Z

src/atoms.jl

+    print(io, "StaticAtom: $(a.element.symbol)")
+end
+
+const AbstractAtomicSystem{D} = AbstractSystem{D,Element}


I don't have time to look for the definition of AbstractSystem, so I'll add this comment here: I can sort of see why one might want to make the dimension a type parameter, though I'm not entirely sure where this is needed?

But does the element type really have to be a type parameter in the abstract type? What is the use case?

cortner · 2021-11-17T17:39:46Z

src/implementation_soa.jl

 #
+# Implementation of AtomsBase interface in a struct-of-arrays style.
+#
+
 using StaticArrays

 export FastSystem


I find the FlexibleSystem and FastSystem terms a bit odd. Isn't it a bit context specific when they are fast / slow / convenient / inconvenient? Why not simply call them SystemSOA and SystemAOS then it is clear what they are. There could be a convenience constructor that can construct either, depending on a flag, with default probable being the AOS but I'm not sure about it.

😆 hahaha that's what they were originally called and I changed them at @mfherbst's suggestion

ah - sorry, that's what I get for not paying attention. Just don't have the time I'm afraid. I can't even quite put my finger on why I don't like those terms. I guess I prefer technically precise terms? Especially with an interface function a user will never have to worry about it until they know what they are doing and then they'll just go and read the docs?

cortner · 2021-11-17T17:41:30Z

src/interface.jl

-#
+Return the velocity of a particle `p`.
+"""
+velocity(p)::Union{Unitful.Velocity,Missing} = missing


could there be also a momentum function that call velocity and mass (and simply fails when there is no mass?)

Certainly...do we think that needs to be at this top level though?

it is so generic and universal that I think there is no harm introducing it? I don't have a strong view - you decide what you think is best.

cortner · 2021-11-17T17:44:38Z

src/interface.jl

@@ -81,82 +38,82 @@ struct DirichletZero <: BoundaryCondition end  # Dirichlet zero boundary (i.e. m
 struct Periodic <: BoundaryCondition end  # Periodic BCs


should this have a flag true/false? Since the bc is implemented as a vector (or tuple) of BCs, you probably want each element of that vector to be e.g. a Periodic() . But now suppose you want a system that is open in the x, y direction and periodic in the z direction (e.g. dislocations). Then you need a different type Open() and you have a type instability. But with Periodic(true), Periodic(false) there is no problem.

Personally, I don't really see how one needs anything other than a boolean here? I can see many other options such as reflecting, embedding, etc but those are not really relevant for the particle system per se, but rather for fields in the background, or for the embedding one would need additional structures anyhow into which to embed the System...

But I don't see any harm in keeping the Periodic type.

cortner · 2021-11-17T17:48:43Z

src/interface.jl

+
+A `D`-dimensional system comprised of particles identified by type `S`.
+"""
+abstract type AbstractSystem{D,S} end


ah - there it is now. So if I want a particle system that has multiple element types then I can't have that? I agree that in 99% of the cases I probably don't want that because it will likely give me type instabilities. Just making sure this is really important enough to sacrifice the flexibility?

I think the group of us has decent consensus that these belong here so I'm going to leave them in when I merge this, but I'll open an issue for further discussion of this point.

cortner · 2021-11-17T17:50:17Z

src/interface.jl

 Base.getindex(::AbstractSystem, ::Int) = error("Implement me")
-Base.size(::AbstractSystem) = error("Implement me")
 Base.length(::AbstractSystem) = error("Implement me")


Is this intended to be implemented now during interface development? Then I think an error message is fine because it will flag that this is supposed to be implemented but isn't. But if it is an interface function that a user should implement, then I don't think the error message should be there.

…and bounding_box for now since there are type annotations... Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin · 2021-11-17T18:08:54Z

I'm going to merge this. I've filed several issues to discuss lingering questions: #25, #26, #27, #28

rkurchin added 2 commits November 10, 2021 16:33

first, just get rid of AbstractElement

c7e8097

Signed-off-by: Rachel Kurchin <[email protected]>

oops, deleted export of AbstractParticle and AbstractAtom in first co…

a615a79

…mmit, put them back Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin force-pushed the less_abstract branch from f7d0a94 to a615a79 Compare November 10, 2021 21:36

rkurchin mentioned this pull request Nov 10, 2021

Do we really need separate AbstractParticle and AbstractElement types? #7

Closed

rkurchin added 2 commits November 10, 2021 16:55

remove AbstractParticle, keep SimpleAtom for now

fede982

Signed-off-by: Rachel Kurchin <[email protected]>

and now AbstractSystem is gone too

7a32c77

Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin added 9 commits November 12, 2021 08:52

Revert "and now AbstractSystem is gone too"

56eea33

This reverts commit 7a32c77.

remove export of ChemicalElement

ce9ac5b

Signed-off-by: Rachel Kurchin <[email protected]>

remove duplicated iterate dispatch

87d9c23

Signed-off-by: Rachel Kurchin <[email protected]>

rename SimpleAtom to StaticAtom, delete phantom implementation_simple…

bfcff94

… file that somehow came back Signed-off-by: Rachel Kurchin <[email protected]>

rename element function to species, add "singular" species function, …

47ae467

…fix return types of other singular functions, rename ET parameter to S, add default size dispatch since firstindex default to a scalar Signed-off-by: Rachel Kurchin <[email protected]>

fix iterate

95c1997

Signed-off-by: Rachel Kurchin <[email protected]>

readme updates, a couple other small fixes

b9bc083

Signed-off-by: Rachel Kurchin <[email protected]>

printing tweaks

8e17ad8

Signed-off-by: Rachel Kurchin <[email protected]>

Michael's suggested change for typing on box

4aca49f

Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin requested review from mfherbst, Gregstrq and jgreener64 and removed request for mfherbst and Gregstrq November 12, 2021 17:16

move atomic stuff to another file, fix some broken functions

9f9743f

Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin force-pushed the less_abstract branch from be999c7 to 9f9743f Compare November 12, 2021 17:45

rkurchin added 3 commits November 12, 2021 13:01

rename get_periodic, add tests

13db820

Signed-off-by: Rachel Kurchin <[email protected]>

add docstrings

89a68ba

Signed-off-by: Rachel Kurchin <[email protected]>

formatting

9f6d7bf

Signed-off-by: Rachel Kurchin <[email protected]>

jgreener64 approved these changes Nov 15, 2021

View reviewed changes

Gregstrq requested changes Nov 16, 2021

View reviewed changes

README.md Show resolved Hide resolved

few tweaks per G's suggestions

9a01ba9

Signed-off-by: Rachel Kurchin <[email protected]>

mfherbst reviewed Nov 17, 2021

View reviewed changes

rkurchin added 2 commits November 17, 2021 09:36

mostly aesthetic changes per Michael's suggestions

54a0a8d

Signed-off-by: Rachel Kurchin <[email protected]>

add indexed versions of position, velocity, species fcns

456a4fe

Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin requested review from Gregstrq and mfherbst November 17, 2021 14:58

rkurchin mentioned this pull request Nov 17, 2021

specific getindex for single vs. slice #8

Closed

This was linked to issues Nov 17, 2021

we should add docstrings #19

Closed

add some more tests #22

Closed

rkurchin mentioned this pull request Nov 17, 2021

Is AbstractElement the best name? #2

Closed

rkurchin linked an issue Nov 17, 2021 that may be closed by this pull request

Do we really need separate AbstractParticle and AbstractElement types? #7

Closed

add tests, fix typo in SoA implementation

0cb1b23

Signed-off-by: Rachel Kurchin <[email protected]>

mfherbst approved these changes Nov 17, 2021

View reviewed changes

cortner reviewed Nov 17, 2021

View reviewed changes

rkurchin mentioned this pull request Nov 17, 2021

Organizing boundary conditions #5

Open

remove getindex and length error throws, leaving boundary_conditions …

f0896f8

…and bounding_box for now since there are type annotations... Signed-off-by: Rachel Kurchin <[email protected]>

rkurchin merged commit b3b7a45 into master Nov 17, 2021

mfherbst deleted the less_abstract branch November 17, 2021 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify types #23

simplify types #23

rkurchin commented Nov 10, 2021

codecov-commenter commented Nov 10, 2021 •

edited

Loading

rkurchin commented Nov 10, 2021

jgreener64 left a comment

Gregstrq left a comment

mfherbst left a comment

mfherbst Nov 17, 2021

rkurchin Nov 17, 2021

jgreener64 Nov 17, 2021

mfherbst Nov 17, 2021

cortner Nov 17, 2021

mfherbst left a comment

cortner left a comment

cortner Nov 17, 2021

cortner Nov 17, 2021

rkurchin Nov 17, 2021

cortner Nov 17, 2021 •

edited

Loading

cortner Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

rkurchin Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

rkurchin Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

rkurchin Nov 17, 2021

cortner Nov 17, 2021

cortner Nov 17, 2021

rkurchin commented Nov 17, 2021

		@@ -81,82 +38,82 @@ struct DirichletZero <: BoundaryCondition end # Dirichlet zero boundary (i.e. m
		struct Periodic <: BoundaryCondition end # Periodic BCs

simplify types #23

simplify types #23

Conversation

rkurchin commented Nov 10, 2021

codecov-commenter commented Nov 10, 2021 • edited Loading

Codecov Report

rkurchin commented Nov 10, 2021

jgreener64 left a comment

Choose a reason for hiding this comment

Gregstrq left a comment

Choose a reason for hiding this comment

mfherbst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfherbst left a comment

Choose a reason for hiding this comment

cortner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cortner Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkurchin commented Nov 17, 2021

codecov-commenter commented Nov 10, 2021 •

edited

Loading

cortner Nov 17, 2021 •

edited

Loading