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

rdf action should be able to return the raw-rdf values #1650

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

DrDomenicoMarson
Copy link
Contributor

Hello everyone,

I just updated the rdf action with an optional boolean argument raw_rdf.
When raw_rdf is set to True, the returned rdf values are not normalized, as the rawrdf argument in cpptraj.

@hainm
Copy link
Contributor

hainm commented Feb 1, 2024

hi @DrDomenicoMarson

Thank you for your PR. Can you please add a quick test for raw_rdf to this file? https://github.com/Amber-MD/pytraj/blob/master/tests/test_analysis/test_rdf.py

Test for RawRDF, from mobile commit
@hainm
Copy link
Contributor

hainm commented Feb 1, 2024

@DrDomenicoMarson Please

git fetch origin
git merge origin/master

to your branch to fix the failed build.

(I have just merged a fix in #1649)

@DrDomenicoMarson
Copy link
Contributor Author

Thanks @hainm for the help!

I added the needed test, do you think this PR will be merged into the upcoming AMBER24 release? To know if I'll be able to switch back to the plain AMBER version of pytraj :-)

Also, I made this PR focusing on maintaining consistent behavior in the pt.rdf() function, that is, one single dataset is always returned. However, when invoked with the rawrdf argument, cpptraj will produce both the raw and normalized data. Now, if the user is interested in both datasets, he/she has to make two pt.rdf() calls.

It would be easy to make pt.rdf() return both the datasets, when called with the raw_rdf argument, but I don't know what is your opinion on having a function that returns a variable number of datasets based on the arguments... please, let me know what you think about it.

@hainm
Copy link
Contributor

hainm commented Feb 1, 2024

do you think this PR will be merged into the upcoming AMBER24 release? To know if I'll be able to switch back to the plain AMBER version of pytraj :-)

@DrDomenicoMarson
yeah, the PR will be merged to the upcoming release of AMBER/AmberTools

@hainm
Copy link
Contributor

hainm commented Feb 1, 2024

Also, I made this PR focusing on maintaining consistent behavior in the pt.rdf() function, that is, one single dataset is always returned. However, when invoked with the rawrdf argument, cpptraj will produce both the raw and normalized data. Now, if the user is interested in both datasets, he/she has to make two pt.rdf() calls.

It would be easy to make pt.rdf() return both the datasets, when called with the raw_rdf argument, but I don't know what is your opinion on having a function that returns a variable number of datasets based on the arguments... please, let me know what you think about it.

I think let's make the return value simple for now (keep the consistent behavior). Thanks.

@hainm hainm merged commit a318898 into Amber-MD:master Feb 2, 2024
1 check passed
@hainm
Copy link
Contributor

hainm commented Feb 2, 2024

Thanks @DrDomenicoMarson very much!

@DrDomenicoMarson
Copy link
Contributor Author

Thanks @DrDomenicoMarson very much!

THanks for the assistance :-)

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