refactor: Argon nodes testing - #2215
Conversation
trisyoungs
left a comment
trisyoungs
left a comment
There was a problem hiding this comment.
Lots happening here! This is probably going to turn into a fairly big PR, but I think it needs to be to capture all the bits that are being changed coherently in one place.
| { | ||
| // Create atom and AtomType | ||
| auto at = atomTypes_.emplace_back(std::make_shared<AtomType>(Z)); | ||
| auto &at = DissolveGraph::addAtomType(atomTypes_, Z); |
There was a problem hiding this comment.
Ah, now, here this is conforming to the old adage of "Mr Dissolve owns it all", whereas now I think AtomTypes need to be stored as local objects on the Species itself (meaning that DissolveNode owns no AtomTypes at all). This way, a Species truly owns all of its data (we'll also need to the same with MasterTerms at some point). How do we share AtomTypes between Species if we do this? We'd need some way of configuring that at the point of creation of the Species, but this is an issue for another day.
| potentialMatrix_[{i, j}] = pp; | ||
| potentialMatrix_[{j, i}] = pp; |
There was a problem hiding this comment.
Interesting - I don't remember storing the full matrix here (i.e. i,j and j,i) since they are very obviously pointing to the same pairpotential. Still valid, however, as the code to lookup an element in a full array is simpler / faster, and this will be called a lot in the energy functions.
46db08a to
e2297c0
Compare
c57ecee to
0dcdb69
Compare
| auto cwd = std::filesystem::current_path(); | ||
| auto path = cwd.parent_path().parent_path() / filename_; |
There was a problem hiding this comment.
We should remove this kind of path-hacking in the code, but I understand why it's here. Since the present example is created programmatically there is no context of a source input file and hence no reference directory. This may be another thing that we have to bring on to the table to discuss sooner rather than later - in #2196 I detailed my thoughts on moving to project folders rather than "bare input files in a random directory" that we have now.
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
6df3566 to
1c620f6
Compare
trisyoungs
left a comment
trisyoungs
left a comment
There was a problem hiding this comment.
One final suggested change, then I think we're good to go!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 8fd9abb | Previous: 7119883 | Ratio |
|---|---|---|---|
BM_Box_MinimumVector<CubicBox> |
11.206905762293081 ns/iter |
5.290696943791192 ns/iter |
2.12 |
BM_Box_MinimumVector<TriclinicBox> |
14.537282484820324 ns/iter |
6.53439697251303 ns/iter |
2.22 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @disorderedmaterials/dissolve-devs
Co-authored-by: Adam Washington <adam.washington@stfc.ac.uk> Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
3 participants
No description provided.