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.

Sign up for GitHub

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

Jump to bottom

Clarify use of null in lattice_vectors #231

Merged
merged 4 commits into from
Dec 24, 2019
Merged

Clarify use of null in lattice_vectors #231

merged 4 commits into from
Dec 24, 2019

Conversation

rartino
Copy link
Contributor

@rartino rartino commented Dec 21, 2019

Fixes #196

@rartino rartino added this to the 1.0 release milestone Dec 21, 2019
ml-evs
ml-evs requested changes Dec 21, 2019
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Minor changes only, please re-request review and I'll accept (can be expedited with a ping on the optimade slack/skype in case I miss the gh email).

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@rartino @ml-evs
Apply suggestions from code review
be515ae 
Co-Authored-By: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@rartino rartino requested a review from ml-evs December 21, 2019 22:48
@rartino rartino added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Dec 21, 2019
ml-evs
ml-evs previously approved these changes Dec 22, 2019
View reviewed changes
CasperWA
CasperWA previously requested changes Dec 22, 2019
View reviewed changes
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Looking great, no content change suggestions here from me.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@CasperWA CasperWA added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing labels Dec 22, 2019
@rartino @CasperWA
Apply suggestions from code review
98db6e5 
Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@rartino rartino added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Dec 23, 2019
merkys
merkys previously approved these changes Dec 23, 2019
View reviewed changes
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

I'm fine with this suggestion.

@rartino rartino requested a review from ml-evs December 23, 2019 16:03
CasperWA
CasperWA requested changes Dec 23, 2019
View reviewed changes
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

An extra space has sneaked its way in to these lines. If this was me, I'm sorry. The problem is that this now produces the "gray indented" rst property, which it shouldn't.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@CasperWA CasperWA added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing labels Dec 23, 2019
@rartino @CasperWA
Fix indentation
92a6abc 
Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@rartino
Copy link
Contributor Author

rartino commented Dec 23, 2019

Weird indentation error, well, it should be fixed now.

@rartino rartino added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Dec 23, 2019
CasperWA
CasperWA approved these changes Dec 24, 2019
View reviewed changes
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Thanks @rartino

@CasperWA CasperWA merged commit ccc26eb into Materials-Consortia:develop Dec 24, 2019
@CasperWA CasperWA mentioned this pull request Jan 6, 2020
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ml-evs ml-evs ml-evs approved these changes

@CasperWA CasperWA CasperWA approved these changes

@merkys merkys merkys left review comments

Assignees
No one assigned
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Milestone
1.0 release
Development

Successfully merging this pull request may close these issues.

Format of 'lattice_vectors' is unclear for 2D, 1D and 0D systems
4 participants
@rartino @ml-evs @merkys @CasperWA