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 amber TI energy output files for testing #6

Merged
merged 6 commits into from
Nov 1, 2017

Conversation

shuail
Copy link
Collaborator

@shuail shuail commented Oct 31, 2017

Fixes #5.

  1. add amber TI output files to test the alchemlyb
  2. modify the MANIFEST.in file to include the .out format (amber output file)

@shuail shuail requested a review from orbeckst October 31, 2017 18:15
@orbeckst
Copy link
Member

orbeckst commented Oct 31, 2017

I updated your comment so that it references #5, the issue that this PR is solving. I will review, hopefully today.

( @dotsdl just in case you have a spare moment and want to do something completely different: I'd be more than happy to have your input, too )

@orbeckst orbeckst self-assigned this Oct 31, 2017
@orbeckst orbeckst mentioned this pull request Oct 31, 2017
@orbeckst
Copy link
Member

I am waiting on PR #7 to be approved. I will then ask you to pick a license for this dataset from recommended open licenses (preferrably CC0 or CC-BY).

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Looks good but please address the following

  1. license (see comments)
  2. add docs:
    • add a file docs/amber.rst (model after gmx.rst
    • add a line amber (after gmx) to file docs/index.rst

:Pressure: 1 bar
:Alchemical Pathway: (charge + vdw) in ligand 1 --> (charge + vdw) in ligand 2, charge and vdw are running in parellel, soft core is used in vdw
:Experimental Free Energy difference: N/A

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line (I know, you copied from the GMX dataset... that line did not serve any purpose there either and I took it out in PR #7)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, will change this together with other comments once I get the license info finalized.


:Missing Values: None
:Date: Oct 2017
:Donor: Silicon Therapeutics
Copy link
Member

Choose a reason for hiding this comment

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

IMPORTANT: add a line

:License: `NAME <URL>`_

that specifies the license that the dataset is released under. See Contributing: Licenses. Specifically, pick a license from the list recommended conformant licenses (with CC0 or CC-BY preferred). You might have to check with your legal department.

In PR #7 I updated the GMX dataset and you can have a look at it to see what I did there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will consult with the company and should get it back soon.

@shuail
Copy link
Collaborator Author

shuail commented Nov 1, 2017

@orbeckst just updated the license info and amber info under docs folder according to the previous comments.

:Missing Values: None
:Date: Oct 2017
:Donor: Silicon Therapeutics
:License: `CC0 <https://creativecommons.org/publicdomain/zero/1.0/>`_ Public Domain Dedication
Copy link
Member

Choose a reason for hiding this comment

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

awesome!

@orbeckst orbeckst merged commit c6b0722 into alchemistry:master Nov 1, 2017
@orbeckst
Copy link
Member

orbeckst commented Nov 1, 2017

Thanks @shuail , merged. (I squashed all your commits into a single one because in this case all commits belonged together and it gets rid of micro-commits such as "fixed typos".)

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.

2 participants