Skip to content

Conversation

@stevenhua0320
Copy link
Contributor

@zmx27 Ready to review

Copy link
Contributor

@zmx27 zmx27 left a comment

Choose a reason for hiding this comment

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

See comments

AUTHORS.rst Outdated
Authors
=======

Simon Billinge, Billinge Group members
Copy link
Contributor

Choose a reason for hiding this comment

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

Xiaohao Yang and Billinge Group members

README.rst Outdated
:target: https://diffpy.github.io/diffpy.srxplanar
:height: 100px

|PythonVersion| |PR|
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this package is open source, I'm guessing that we are likely going to be releasing the package to PyPI/CF? Let's ask Professor Simon to be sure. If so, you can add back the CI and CF, etc flags that skpkg generates

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this one can go to pypi and conda-forge

README.rst Outdated
.. |Tracking| image:: https://img.shields.io/badge/issue_tracking-github-blue
:target: https://github.com/diffpy/diffpy.srxplanar/issues

Distance Printer, calculate the inter atomic distances. Part of xPDFsuite
Copy link
Contributor

Choose a reason for hiding this comment

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

Change description

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to raise this again. I think this applies to distanceprinter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it should not be here, but originally it is in its description in the project.toml. It is a little bit weird, but I would delete it and retain the other description of this package.

README.rst Outdated
Citation
--------

If you use diffpy.srxplanar in a scientific publication, we would like you to cite this package as
Copy link
Contributor

Choose a reason for hiding this comment

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

The citation info is already in the file. It should be referring to the paper, so maybe move that section here instead?

------------

If your python version < 2.7 (these two packages are included in 2.7 but not in 2.6)
The preferred method is to be installed with `xpdfsuite` package or the wheel file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but if we do end up releasing this package to PyPI/CF, we can also mention how we can install it from there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zmx27 Professor said we do the package in private repo way yesterday. So, I think we do not need to release it to PyPI/CF?

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Nitpicking here, but maybe we can also add in some instructions on how to install it as a wheel file

.. |Black| image:: https://img.shields.io/badge/code_style-black-black
:target: https://github.com/psf/black

.. |Codecov| image:: https://codecov.io/gh/diffpy/diffpy.srxplanar/branch/main/graph/badge.svg
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not releasing to PyPI/CF like you said, we should be good to delete the Codecov, PyPI icons, etc. Anything that we are not using like Black

------------

If your python version < 2.7 (these two packages are included in 2.7 but not in 2.6)
The preferred method is to be installed with `xpdfsuite` package or the wheel file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Nitpicking here, but maybe we can also add in some instructions on how to install it as a wheel file

README.rst Outdated
Xiaohao Yang, Pavol Juhas, Simon J. L. Billinge, On the estimation of
statistical uncertainties on powder diffraction and small angle
scattering data from 2-D x-ray detectors, arXiv:1309.3614
software.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit unclear to me, but this paragraph ends with If you are using this software.?

README.rst Outdated

|PythonVersion| |PR|

|Tracking|
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I realized now that my wording was misleading. We do want things like PythonVersion, PR, Tracking, and Black, as well as their associated icons

README.rst Outdated
.. |Tracking| image:: https://img.shields.io/badge/issue_tracking-github-blue
:target: https://github.com/diffpy/diffpy.srxplanar/issues

Distance Printer, calculate the inter atomic distances. Part of xPDFsuite
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to raise this again. I think this applies to distanceprinter?

@stevenhua0320 stevenhua0320 requested a review from zmx27 October 23, 2025 14:41
Copy link
Contributor

@zmx27 zmx27 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

@stevenhua0320
Copy link
Contributor Author

@sbillinge Ready to review

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.

pls see one comment. Also, on a different PR we will need workflows that do releases to pypi like in scikit package, not like pdfgetx in this case.

README.rst Outdated
:target: https://diffpy.github.io/diffpy.srxplanar
:height: 100px

|PythonVersion| |PR|
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this one can go to pypi and conda-forge

@stevenhua0320
Copy link
Contributor Author

pls see one comment. Also, on a different PR we will need workflows that do releases to pypi like in scikit package, not like pdfgetx in this case.

OK, I would make edition to all these files in two PRs.

@stevenhua0320
Copy link
Contributor Author

@sbillinge Ready for README.rst another review

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.

pls see comment

LICENSE.rst Outdated
BSD 3-Clause License

Copyright (c) 2008-2025, The Trustees of Columbia University in the City of New York.
Copyright (c) 1994-2025, Christoph Gohlke
Copy link
Contributor

Choose a reason for hiding this comment

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

Did Christoph Gohlke make edits all the way to this year? I don't know what he contributed, but in any case, leave the dates as they were.

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