Skip to content

refactor: Separate histogram calculation from PartialSet - #2216

Merged
trisyoungs merged 4 commits into
develop2from
dissolve2/split-partialset
Jul 31, 2025
Merged

refactor: Separate histogram calculation from PartialSet#2216
trisyoungs merged 4 commits into
develop2from
dissolve2/split-partialset

Conversation

@trisyoungs

Copy link
Copy Markdown
Member

This PR splits the histograms for radial distribution function calculation out of PartialSet into their own HistogramSet since this represented a very specific set of data contained in a quite general-use class otherwise.

Required by #2203.

@RobBuchananCompPhys RobBuchananCompPhys left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a couple of comments including a suggestion that may/may not be worth implementing.

Comment on lines +55 to +60
for (auto &histo : fullHistograms_.linearArray())
histo.zeroBins();
for (auto &histo : boundHistograms_.linearArray())
histo.zeroBins();
for (auto &histo : unboundHistograms_.linearArray())
histo.zeroBins();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of a static method in Array2D, flatten, that takes a couple of Array2D<T>'s (like the above full/bound/unboundHistograms_) and flattens their linearArray()s into a single vector?

Suggested change
for (auto &histo : fullHistograms_.linearArray())
histo.zeroBins();
for (auto &histo : boundHistograms_.linearArray())
histo.zeroBins();
for (auto &histo : unboundHistograms_.linearArray())
histo.zeroBins();
for (auto &histo : Array2D::flatten({fullHistograms_, boundHistograms_, unboundHistograms_}))
histo.zeroBins();

I believe the helper method could be implemented like:

static std::vector<A> flatten(const std::vector<Array2D<A>> arrays)
{
    std::vector<A> flat;
    for (const auto &a : arrays)
          flat.insert(flat.end(), a.begin(), a.end());
    return flat;
} 

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean - in fact Adam's zip() operator would allow us to do this in one loop, albeit with three variables. Your suggestion makes a copy of the data, so unfortunately wouldn't have the intended result. I think I will leave this as-is for now, but your suggestion makes me think that some kind of function which takes a lambda and operates on every element in the Array2D would be useful to implement in the future (e.g. fullHistograms_.operate([](auto &histo) { histo.zeroBins(); }))

Comment thread src/classes/histogramSet.h
@trisyoungs
trisyoungs merged commit 88da25c into develop2 Jul 31, 2025
11 checks passed
@trisyoungs
trisyoungs deleted the dissolve2/split-partialset branch July 31, 2025 12:34
rprospero pushed a commit that referenced this pull request Sep 5, 2025
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
rprospero pushed a commit that referenced this pull request Feb 4, 2026
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants