replace HalfspaceIntersection import and pin scipy >= 1.8.0, sort imports

[DanielYang59]

• replace HalfspaceIntersection import and pin scipy >= 1.8.0, getting:

  venv/lib/python3.12/site-packages/pymatgen/analysis/alloys/core.py:28
    /Users/yang/developer/api/venv/lib/python3.12/site-packages/pymatgen/analysis/alloys/core.py:28: DeprecationWarning: Please import `HalfspaceIntersection` from the `scipy.spatial` namespace; the `scipy.spatial.qhull` namespace is deprecated and will be removed in SciPy 2.0.0.
      from scipy.spatial.qhull import HalfspaceIntersection
  

• sort imports

[DanielYang59]

@tschaume Perhaps you could review this as well? Thanks!

[tsmathis]

@DanielYang59, should a minimum version of scipy be specified in the setup.py for this change?

[DanielYang59]

Sorry it seems I pinged the wrong person 😅

should a minimum version of scipy be specified in the setup.py for this change?

Yes that's a solid point, scipy 1.8.0 is the version that deprecated access to these private APIs so I believe it would be safe to pin scipy>=1.8.0 (pymatgen now requires scipy>=1.13.0 so there wouldn't be conflicts).

Meanwhile I noticed many external packages (networkx, numpy, pandas, plotly, monty, scipy) are not listed for some reason? I sorted the imports to make these more visible.

pymatgen-analysis-alloys/setup.py

Line 21 in 75a88e7

install_requires=["pymatgen>=2023.7.17", "shapely>=1.8.2"],

[tsmathis]

No worries about the ping, I was just added recently to help spread the maintenance workload around for the MP team

That said, this PR has spread quite a bit beyond the initial scope of the original deprecated scipy import into packaging, workflow management, and also code style? A cursory check looks like these changes are okay, but in the future please raise these sorts of "housekeeping" changes as either issues or more targeted PRs. I do appreciate the enthusiasm though.

And in the spirit of housekeeping and maintenance, since this repo is not under active development, and unless that changes, we will likely only be considering contributions that help to maintain the stability of the package.

[tsmathis]

Forgot to ask, would you mind changing the title + description to better match the current scope of your PR?

[DanielYang59]

That said, this PR has spread quite a bit beyond the initial scope of the original deprecated scipy import into packaging, workflow management, and also code style? A cursory check looks like these changes are okay, but in the future please raise these sorts of "housekeeping" changes as either issues or more targeted PRs. I do appreciate the enthusiasm though.

Sorry for the mess. I would migrate these unrelated changes to another PR :)

[tsmathis]

No need to apologize, and it’s fine to keep everything as is in this PR. Just asking to keep it in mind for future contributions to this repo to keep the overhead low for maintenance.

…

On Tue, Oct 8, 2024 at 6:51 PM Haoyu (Daniel) YANG ***@***.***> wrote: That said, this PR has spread quite a bit beyond the initial scope of the original deprecated scipy import into packaging, workflow management, and also code style? A cursory check looks like these changes are okay, but in the future please raise these sorts of "housekeeping" changes as either issues or more targeted PRs. I do appreciate the enthusiasm though. Sorry for the mess. I would migrate these unrelated changes to another PR :) — Reply to this email directly, view it on GitHub <#6 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIPH7ACGCEHDQIXS2PXA5QLZ2SDYJAVCNFSM6AAAAABPG2HSKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRGEYTEOBYGQ> . You are receiving this because you commented.Message ID: ***@***.***>

[DanielYang59]

Just asking to keep it in mind for future contributions to this repo to keep the overhead low for maintenance.

Of course! I would keep the sorting of imports in this PR (otherwise it's hard to see what external packages are required) and addition of install_requires in setup.py, otherwise it's not possible to pin scipy version as it's not listed in the setup.py.

[tsmathis]

That is fine by me. And since I see you are choosing to revert some of your commits, when you are satisfied with the state of your changes please rebase your changes to keep the git history clean. When you are happy with the changes just leave a message that you are done making changes. If you do finish today, I will go ahead with merging this tomorrow and try getting a new release up on pypi.

…

On Tue, Oct 8, 2024 at 7:25 PM Haoyu (Daniel) YANG ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In setup.py <#6 (comment)> : > extras_require={}, + setup_requires=["setuptools>=64"], I might keep this line of change in this PR, as setuptools < 58.3.0 is known to have buggy behaviour with package data handling <https://github.com/abravalheri/experiment-setuptools-package-data> — Reply to this email directly, view it on GitHub <#6 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIPH7AF3LZ7FU7YKTRR4ZBLZ2SHZNAVCNFSM6AAAAABPG2HSKKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNJVHAZDOOJXHA> . You are receiving this because you commented.Message ID: ***@***.*** .com>

[DanielYang59]

And since I see you are choosing to revert some of your commits, when you
are satisfied with the state of your changes please rebase your changes to
keep the git history clean.

I don't think I would make further commit to the PR. Can you perhaps squash all commits? Thanks for your time.

[shyuep]

The reason why some external packages are not specified is because this is a add-on package to pymatgen. Pymatgen is a requirement and already sets a lot of the package requirements (e.g., numpy, scipy, etc.). Specifying these versions would create a maintenance headache because there can be conflict between the pymatgen version and the addon specification. So unless there is a good reason to peg to a different version from pymatgen, we just specify the pymatgen version.

[tsmathis]

Thanks for weighing in @shyuep, definitely makes a lot of sense.

@DanielYang59, in light of that, I am going to close this PR due to the git history and I will fix the deprecated scipy import that you first reported as a follow up

[tsmathis]

closing in favor of #8

[DanielYang59]

Hi thanks for the comment @shyuep I appreciate that.

Specifying these versions would create a maintenance headache because there can be conflict between the pymatgen version and the addon specification.

This PR doesn't pin specific versions of those newly added external packages so I don't think there would be version conflicts.

So unless there is a good reason to peg to a different version from pymatgen, we just specify the pymatgen version.

Good to know, admittedly this is a good way to include dependency without duplicate as it's intended as an add-on package (thought it's just a standalone namespace package).

As the scipy pin is compatible with pymatgen, let's pin it there.