Skip to content

Conversation

@bobleesj
Copy link
Contributor

Please review. @sbillinge src/ and doc/ will be handled in the next PRs.

Copy link
Contributor Author

@bobleesj bobleesj left a comment

Choose a reason for hiding this comment

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

@sbillinge Please review.

Unnecessary files will be also removed in the upcoming PRs.

LICENSE.rst Outdated
@@ -0,0 +1,30 @@
BSD 3-Clause License

Copyright (c) 2009-2024, The Trustees of Columbia University
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2009-2024 added

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this work does not go back to 2009

Contact
-------

For more information on diffpy.nmf_mapping please visit the project `web-page <https://diffpy.github.io/>`_ or email Prof. Simon Billinge at sb2896@columbia.edu.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

diffpy.nmf_mapping configured correctly.

@bobleesj bobleesj marked this pull request as ready for review August 11, 2024 00:50
@sbillinge
Copy link
Contributor

Looks good. Just some issues with copyright dates maybe?

@bobleesj
Copy link
Contributor Author

Looks good. Just some issues with copyright dates maybe?

d40b807

I see the first commit being 2022. I will make a change.

Copy link
Contributor Author

@bobleesj bobleesj left a comment

Choose a reason for hiding this comment

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

Please check the new copyright years

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Please see comments

README.rst Outdated

If you use diffpy.nmf_mapping in a scientific publication, we would like you to cite this package as

diffpy.nmf_mapping Package, https://github.com/diffpy/diffpy.nmf_mapping
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a paper/papers that need citing for this one. Also in the docs

Copy link
Contributor Author

@bobleesj bobleesj Aug 11, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-08-11 at 10 54 02 AM

Fixed in REAMDE.rst. I will add citations to docs in the following PRs where I move from docs to doc using git mv. @sbillinge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Citation source: https://pdfitc.org/NMF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the citation format, in diffpy.structure and diffpy.pdffit2. I will make a change.

Screenshot 2024-08-11 at 11 04 40 AM Screenshot 2024-08-11 at 11 04 48 AM

@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 11, 2024

@sbillinge I also have a general question on a GH workflow.

Assume I made a mistake in a commit, and I want to revert my commit (to fix the problem). I also do not want the reviewer to waste time reading the mistake.

I see some options such as 1) force push 2) git revert. I see that git revert indicates a clear undoing of the previous changes. I feel uncomfortable with the word "force".

What is the best practice?

@bobleesj
Copy link
Contributor Author

Please review:

Citations added to README.rst, URLs working.

Screenshot 2024-08-11 at 11 17 50 AM

@sbillinge
Copy link
Contributor

Please review:

Citations added to README.rst, URLs working.

Screenshot 2024-08-11 at 11 17 50 AM

Nice job. For the second paper add the words "and please consider citing " before it.

@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 11, 2024

Nice job. For the second paper add the words "and please consider citing " before it.

Yup. Added. I will reflect those in doc as well in the following PR.

Screenshot 2024-08-11 at 4 07 35 PM

@sbillinge
Copy link
Contributor

@sbillinge I also have a general question on a GH workflow.

Assume I made a mistake in a commit, and I want to revert my commit (to fix the problem). I also do not want the reviewer to waste time reading the mistake.

I see some options such as 1) force push 2) git revert. I see that git revert indicates a clear undoing of the previous changes. I feel uncomfortable with the word "force".

What is the best practice?

never force push. I don't want to merge any force-pushes. Some repo's have this set as a rule. just revert if it is a small commit. The revert applies the anti-change, so the change is done then undone, which is ok if it is small, but if it is big it is generally better to kill the branch and do a new PR. Again, another reason to keep the edits on each PR to be small.

@sbillinge sbillinge merged commit d677d87 into diffpy:main Aug 11, 2024
@bobleesj
Copy link
Contributor Author

never force push. I don't want to merge any force-pushes. Some repo's have this set as a rule. just revert if it is a small commit. The revert applies the anti-change, so the change is done then undone, which is ok if it is small, but if it is big it is generally better to kill the branch and do a new PR. Again, another reason to keep the edits on each PR to be small.

Thank you. Well noted. @sbillinge

@bobleesj bobleesj deleted the cookierelease branch August 11, 2024 20:54
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