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 new restraints Application layer interface #1038

Merged
merged 3 commits into from
Jul 26, 2018

Conversation

Lnaden
Copy link
Contributor

@Lnaden Lnaden commented Jul 16, 2018

Note: This PR is being merged into the new multirestraint branch. Much like the SAMS/MultiStateSampler branch, I wanted to make a branch we can all work on that is not master as we start this process, comment, and review as we would normal PRs. I have not configured travis to run on this branch yet, but my local tests are passing.

  • Added new restraints block to the YAML
    • Exactly the same as it used to be in experiments block but with unique ID string
    • Added new name key which can be used to target custom lambda_name variables, optional and defaults to restraints. Non-unique to let multiple restraints be controlled by same variable.
    • Block is 100% optional
  • experiments block allows restraint: target to target restraint in restraints block.
    • Supports old style to preserve backwards compatibility
    • Supports targeting a single restraint from the restraints block
    • Supports a list of targets in the restraints block to use multi-restraints
  • Updated and added tests for new application layer feature
    • Note: Tests should pass but there is no underlying mechanism to use this yet
  • Updated docs
    • New restraints doc page
    • Updated experiments page with behavior changes to the restraint keyword
    • Updated index pages to include new page and pointers.

A key thing: These application layer changes should be 100% compatible with the older versions. So we should not have to have users change their YAML files like we did with the MultiStateSampler conversion. That said, this is still a major new feature and the start of the 0.24.0 release.

* Added new `restraints` block to the YAML
    * Exactly the same as it used to be in `experiments` block but with unique ID string
    * Added new `name` key which can be used to target custom `lambda_name` variables, optional and defaults to `restraints`. Non-unique to let multiple restraints be controlled by same variable.
    * Block is 100% optional
* `experiments` block allows `restraint: target` to target restraint in `restraints` block.
    * Supports old style to preserve backwards compatibility
    * Supports targeting a single restraint from the `restraints` block
    * Supports a list of targets in the `restraints` block to use multi-restraints
* Updated and added tests for new application layer feature
    * Note: Tests should pass but there is no underlying mechanism to use this yet
* Updated docs
    * New `restraints` doc page
    * Updated `experiments` page with behavior changes to the `restraint` keyword
    * Updated index pages to include new page and pointers.

A key thing: These application layer changes should be 100% compatible with the older versions. So we should not have to have users change their YAML files like we did with the MultiStateSampler conversion. That said, this is still a major new feature and the start of the 0.24.0 release.
@Lnaden Lnaden requested review from andrrizzi and jchodera July 16, 2018 18:35
@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 16, 2018

This is also progress on #1036 and #1015

@jchodera
Copy link
Member

Thanks for taking a stab at this, @Lnaden!

When extending the YAML syntax, it helps to add some concrete examples instead of just defining the schemas. Could you draft an example YAML file that illustrates the new structure you envision here?

Copy link
Contributor

@andrrizzi andrrizzi 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!

Let's make sure to remove the line that pops the name out of the restraint constructor once we add the feature.

@andrrizzi
Copy link
Contributor

Ah! Actually, I think this PR has the same fault as the samplers and mcmc_moves section, and it doesn't get saved in the generated YAML file (see #947). We can correct it in a separate PR if you prefer.

@jchodera
Copy link
Member

@Lnaden: Repeating this here, since I think it was overlooked:

Could you draft an example YAML file that illustrates the new structure you envision here?

The PR doesn't include an actual example of what you are envisioning the new YAML syntax to look like for a real project that uses this feature.

@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 17, 2018

I was including this in the cookbook changes I was doing as well. Sorry for not explicitly mentioning it here!

@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 17, 2018

Let's make sure to remove the line that pops the name out of the restraint constructor once we add the feature.

That's on purpose. It puts the name right back on it after it calls the constructor.

I think this PR has the same fault as the samplers and mcmc_moves section,

Good catch, I'll fix that

@andrrizzi
Copy link
Contributor

That's on purpose. It puts the name right back on it after it calls the constructor.

I meant that when name will be an argument of the restraints' constructor, we should remove it. Unless I misremember the plan and name won't be an __init__ parameter.

@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 18, 2018

name will be part of the constructors. We need a CV variable name, and restraints is going to be the default.

@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 18, 2018

Thats also why I am proposing this into the multirestraint branch and not master, because I know it will break things because this pr is just not ready yet.

Output YAML now includes samplers, mcmc_moves, and restraints blocks (Fixes #947)
Fixed typo in copy pate of the mcmc example
@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 26, 2018

I have made a couple changes to this part of the multi-restraints.

@jchodera I have added a hard example YAML showing the new format here as part of the docs

@andrrizzi I should have fixed #947 with this, could you please review the diff and make sure it looks good to you? This will include the mcmc_moves, samplers and restraints blocks only if they are needed. I have tested them all locally. I also kind of generalized the write-out code in case we need to make some changes to it in the future.

Copy link
Contributor

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Looks great!


# Export YAML into a file
import pdb; pdb.set_trace()
Copy link
Contributor

Choose a reason for hiding this comment

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

Found it :) !

@Lnaden
Copy link
Contributor Author

Lnaden commented Jul 26, 2018

I always miss the 1. Thank you!

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.

3 participants