Skip to content
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

Refactor RDF as an observable #3706

Merged
merged 15 commits into from
May 15, 2020
Merged

Refactor RDF as an observable #3706

merged 15 commits into from
May 15, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented May 6, 2020

Folllow-up to #3599

Description of changes:

  • replaced the RDF analysis tool by an RDF observable
  • removed the particle position storage mechanism from statistics.cpp
  • documented observable TimeSeries

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@jngrad
Copy link
Member Author

jngrad commented May 6, 2020

The RDF observable is kind of a stand-alone class. If we decide to port calc_part_distribution to an observable (which is very similar to RDF) in the future, we might create an intermediate observable class that takes two lists of particle ids, but for now I'm somewhat ok with this architecture.

The time series documentation needs to be backported in 4.1.3.

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #3706 into python will decrease coverage by 0%.
The diff coverage is 87%.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3706   +/-   ##
======================================
- Coverage      88%     88%   -1%     
======================================
  Files         545     549    +4     
  Lines       24721   24747   +26     
======================================
+ Hits        21779   21787    +8     
- Misses       2942    2960   +18     
Impacted Files Coverage Δ
src/core/statistics.cpp 67% <ø> (-11%) ⬇️
src/script_interface/observables/RDF.hpp 45% <45%> (ø)
src/core/observables/RDF.hpp 77% <77%> (ø)
src/core/observables/PidObservable.cpp 100% <100%> (ø)
src/core/observables/RDF.cpp 100% <100%> (ø)
src/core/observables/fetch_particles.hpp 100% <100%> (ø)
src/script_interface/observables/initialize.cpp 100% <100%> (ø)
src/utils/include/utils/for_each_pair.hpp 100% <100%> (ø)
src/utils/tests/for_each_pair_test.cpp 100% <100%> (ø)
src/core/particle_data.cpp 95% <0%> (-1%) ⬇️
... and 5 more

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 ff10b53...02af73a. Read the comment docs.

}
while (first1 != last1) {
for (auto it = first2; it != last2; ++it) {
if (*it != *first1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that comparison by value is what we want here. This limits the usability to value types that are comparable without need, and the behavior is different from for_each (which does it != first, e.g. shallow comparison), which is surprising. I think in the general algorithm the pairs should not be filtered, this can be implemented in the op. One could also implement a for_each_pair_if on if wanted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was unsure about that solution too. If we remove that conditional, then we cannot decay this function to for_each_pair, since that one removes diagonal elements. In fact, we probably don't want to decay in any case, because for_each_pair also removes the symmetric pairs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've coded a for_each_cartesian_pair_if and removed the decay to for_each_pair.

@fweik fweik self-assigned this May 9, 2020
@fweik fweik requested review from KaiSzuttor and removed request for RudolfWeeber May 9, 2020 18:37
@fweik
Copy link
Contributor

fweik commented May 14, 2020

@KaiSzuttor did you have time to look at this?

Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

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

LGTM except for some small tweaks.

@jngrad jngrad added the automerge Merge with kodiak label May 15, 2020
@kodiakhq kodiakhq bot merged commit 190ca01 into espressomd:python May 15, 2020
@jngrad jngrad deleted the rdf branch January 18, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants