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

Add radians option to angle_between_vectors and add value ranges to bond distribution functions. #35

Merged
merged 13 commits into from
Feb 28, 2022

Conversation

chrisjonesBSU
Copy link
Member

This PR does a couple of things:

Adds a parameter so that geometry.angle_between_vectors can return the angle in degrees or radians. It defaults to degrees

Adds a radians option to the bond and angle distribution functions.

Adds min and max options to the distribution functions, so that it can work similar to the gsd_rdf function

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #35 (75922d0) into master (e96dd7f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   99.68%   99.69%           
=======================================
  Files           7        7           
  Lines         318      328   +10     
=======================================
+ Hits          317      327   +10     
  Misses          1        1           
Impacted Files Coverage Δ
cmeutils/geometry.py 100.00% <100.00%> (ø)
cmeutils/plotting.py 100.00% <100.00%> (ø)
cmeutils/structure.py 100.00% <100.00%> (ø)

Copy link
Member

@jennyfothergill jennyfothergill left a comment

Choose a reason for hiding this comment

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

Might want to add tests for theta_min/theta_max and l_min/l_max just to validate that these values don't break anything. Also what happens if the min and max are set such that they are outside the distribution?

Comment on lines 95 to 96
else:
return angle
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else:
return angle
return angle

cmeutils/geometry.py Outdated Show resolved Hide resolved
@chrisjonesBSU
Copy link
Member Author

chrisjonesBSU commented Feb 28, 2022

Might want to add tests for theta_min/theta_max and l_min/l_max just to validate that these values don't break anything. Also what happens if the min and max are set such that they are outside the distribution?

Yeah, I can add a test. To your question, that's kind of the point of having the min and max values. For example, say some bond distribution only has values between 1 and 2, but you want the distribution to go from zero to 3, so you'd set min=0, max=3 and the distribution would include the entire range from min to max, even if a good portion of it is zero. Unless I'm not understanding what you mean by outside the distribution.

@jennyfothergill
Copy link
Member

Might want to add tests for theta_min/theta_max and l_min/l_max just to validate that these values don't break anything. Also what happens if the min and max are set such that they are outside the distribution?

Yeah, I can add a test. To your question, that's kind of the point of having the min and max values. For example, say some bond distribution only has values between 1 and 2, but you want the distribution to go from zero to 3, so you'd set min=0, max=3 and the distribution would include the entire range from min to max, even if a good portion of it is zero. Unless I'm not understanding what you mean by outside the distribution.

What I mean using your example of a bond distribution with values between 1 and 2, what if you set the min and max to 1.5 to 3? Some of the distribution isn't captured. Is there anything we should do to handle that? (I don't mean these as leading questions, I'm just thinking about possible hangups.)

@chrisjonesBSU
Copy link
Member Author

If that's the case then I'm pretty sure numpy.histogram just trims the distribution based on the range you give it. We could maybe add a warning if any part of the array of values returned (bond lengths, angles) lies outside of either min or max values, but it wouldn't error out.

Copy link
Member

@jennyfothergill jennyfothergill left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@chrisjonesBSU chrisjonesBSU merged commit a7695c6 into cmelab:master Feb 28, 2022
@chrisjonesBSU chrisjonesBSU deleted the bond_dist_range branch February 28, 2022 23:05
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