Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
simplify types #23
simplify types #23
Changes from all commits
c7e8097
a615a79
fede982
7a32c77
56eea33
ce9ac5b
87d9c23
bfcff94
47ae467
95c1997
b9bc083
8e17ad8
4aca49f
9f9743f
13db820
89a68ba
9f6d7bf
9a01ba9
54a0a8d
456a4fe
0cb1b23
f0896f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"morally" it seems to me that
box
andboundary_conditions
should be tuples of ... rather than SVectors. But I don't have too strong a view on that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I did it in JuLIP was to have a conversion interface e.g.
and so forth. So this means that ANY sensible input will be automatically converted into the format needed internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will repost in the issue.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the
FlexibleSystem
andFastSystem
terms a bit odd. Isn't it a bit context specific when they are fast / slow / convenient / inconvenient? Why not simply call themSystemSOA
andSystemAOS
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 hahaha that's what they were originally called and I changed them at @mfherbst's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?