Skip to content

Conversation

bobleesj
Copy link
Contributor

No description provided.

from diffpy.pdffit2.version import __version__, __date__
from diffpy.pdffit2.pdffit import PdfFit
from diffpy.pdffit2.pdffit2 import is_element
from diffpy.pdffit2.version import __date__, __version__
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 finally found one origin of "import" problem.

The C++ extension below pdffit2.pdffit2 uses the __verison__ global variable.

from diffpy.pdffit2.pdffit2 import is_element

But when we apply isort via pre-commit, the C++ module is imported first before __version__ variable is initialized, causing the import issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good work @bobleesj . I guess we need a fix? Is it to disable isort for this case? Or change how the imports are done? It is a bit brittle if it can be broken by isort, but that fix is ok if we document it carefully so we don't break it again in the first when we try and do better linting....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(thinking about a solution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbillinge Indeed there are two options. First, we can disable isort for this file using .flake8 (the previous solution), but this approach is brittle since the .flake8 file can be overwritten when reapplying the cookiecutter template, as you mentioned.

The second option is to disable isort at the file level using the isort: off/on directives, as shown here. I've also made the comments more explicit to ensure that these directives shall not be removed.

@bobleesj bobleesj mentioned this pull request Sep 24, 2024
@bobleesj
Copy link
Contributor Author

Ready for review @sbillinge !

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 inline

.coveragerc Outdated
omit =
# exclude debug.py from codecov report
*/tests/debug.py
diffpy.structure
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be diffpy.pdffit2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

uses: Billingegroup/release-scripts/.github/workflows/_matrix-and-codecov-on-merge-to-main.yml@v0
with:
project: diffpy.structure
c_extension: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check. I think this maybe should be yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

types:
- prereleased
- published
workflow_dispatch: null
Copy link
Contributor

Choose a reason for hiding this comment

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

Still this null bug.

http://www.diffpy.org/doc/pdffit2.

For more information about the diffpy.pdffit2 library, please consult our `online documentation <https://diffpy.github.io/diffpy.pdffit2>`_.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please look for a rogue slash in the curation below.

Also, please replace the installation instructions with the cookie cutter version. If there are special instructions for installing it from sources we can add them, but use the cookie cutter as a starting point with minimal updates and delete the old instructions.

Copy link
Contributor Author

@bobleesj bobleesj Sep 24, 2024

Choose a reason for hiding this comment

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

Please look for a rogue slash in the curation below.

Removed

Also, please replace the installation instructions with the cookie cutter version

Indeed the starting point was the cookiecutter. It was further populated from the prior readme.rst (confirmed since all the matrix yml links above modified automatically)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we can make an issue for now to improve the installation instructions in reaadme once we have a PyPI/conda version available?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, do it now. We won't release until we have a puppy/cons version and then the instructions will work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instructions added.

Copy link
Contributor Author

@bobleesj bobleesj Sep 24, 2024

Choose a reason for hiding this comment

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

from diffpy.pdffit2.version import __version__, __date__
from diffpy.pdffit2.pdffit import PdfFit
from diffpy.pdffit2.pdffit2 import is_element
from diffpy.pdffit2.version import __date__, __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

Good work @bobleesj . I guess we need a fix? Is it to disable isort for this case? Or change how the imports are done? It is a bit brittle if it can be broken by isort, but that fix is ok if we document it carefully so we don't break it again in the first when we try and do better linting....

we ask that you acknowledge use of the program by citing the following paper
in your publication:

C. L. Farrow, P. Juhás, J. W. Liu, D. Bryndin, E. S. Božin, J. Bloch, Th. Proffen
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rogue slash removed.

@bobleesj
Copy link
Contributor Author

@sbillinge Ready for 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.

Please see inline


To confirm that the installation was successful, run the following in a terminal ::
python -c "import diffpy.pdffit2; print(diffpy.pdffit2.__version__)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a quick version test for the user.

@bobleesj
Copy link
Contributor Author

@sbillinge ready for review - primarily pip install instruction

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.

one small last tweak

To install using ``pip`` into your ``diffpy.pdffit2_env`` environment, we will also have to install dependencies ::

pip install -r https://github.com/raw/diffpy/diffpy.pdffit2/main/requirements/run.txt
To install the C++ compiler and required dependencies in your ``diffpy.pdffit2_env`` environment, type ::
Copy link
Contributor

Choose a reason for hiding this comment

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

The less preferred approach is the install using ``pip`` to download and install the latest release from
`Python Package Index <https://pypi.python.org>`_.  In this case you currently have to build the codes on your computer from the sources and you will have to have a C++ installer available.
To install the C++ .....
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@bobleesj
Copy link
Contributor Author

@sbillinge Ready for review - just modified the pip source install instruction suggested by you.

@sbillinge sbillinge merged commit f04c782 into diffpy:main Sep 24, 2024
2 checks passed
@bobleesj bobleesj deleted the recookiecut branch September 24, 2024 20:00
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