Skip to content

fix version-string (PEP compliant) and other stuff #134

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

Open
YAY-C wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix version-string (PEP compliant) and other stuff #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3e8e0f
drity fix for version-ID PEP compliance
YAY-C Dec 15, 2025
9df83a3
fixed submodule `__init__`
YAY-C Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions qstack/spahm/rho/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
"""Atom- and bond-based SPAHM module."""

from . import compute_rho_spahm
Copy link
Contributor Author

@YAY-C YAY-C Dec 15, 2025

Choose a reason for hiding this comment

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

this is necessary to expose the underlying functions when importing qstack.spahm.rho
@liam-o-marsh @briling do you normally import main python script or rather import all subscripts individually?

Copy link
Contributor

@briling briling Dec 15, 2025

Choose a reason for hiding this comment

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

what's the problem here?
i wanted to be able to run python -m qstack.spahm.rho instead of python -m qstack.spahm.rho.compute_rho_spahm

Copy link
Contributor Author

@YAY-C YAY-C Dec 15, 2025

Choose a reason for hiding this comment

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

Yes, that part works perfectly, thanks to the __main__.py script.
However, import qstack.spahm.rho in any script outside the Q-stack tree does not allow you to import the functions/scripts in that folder, only an empty module!
The changes I made resolve the issue, since the compute_rho_spahm.py script basically imports all the other functions in the folder (to be verified), but I was wondering if that was enough or if we should import all the scripts in the folder individually in __init__.py
i.e.

from . import Dmatrix, atomic_density, bond_selected, compute_rho_spahm, dmb_rep_atom, dmb_rep_bond, lowdin, parser, sym, utils

I tried to check how you did it in the other folders, but it didn't seem systematic.

Copy link
Contributor

@briling briling Dec 15, 2025

Choose a reason for hiding this comment

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

oh I see now
do we need all the modules to be imported though?

Copy link
Contributor

@liam-o-marsh liam-o-marsh Dec 17, 2025

Choose a reason for hiding this comment

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

hm...
I see a different problem:
I imagine people would want to import qstack.spahm.rho to call the get_repr or spahm_a_b functions in compute_rho_spahm.py, but since that file is a script it's better to not import it. (due to the self-importing shenanigans that will occur when calling the script itself. Assuming we want the script itself to be the right way to call it. Technically the docs recommend calling python3 -m qstack.spahm.rho instead)

Copy link
Contributor

@briling briling Dec 17, 2025

Choose a reason for hiding this comment

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

but I thought import qstack.spahm.rho imports __init__.py and the script is in __main__.py

Copy link
Contributor

@liam-o-marsh liam-o-marsh Dec 17, 2025

Choose a reason for hiding this comment

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

yes, but if __init__ imports compute_rho_spahm, and somebody tries to run compute_rho_spahm.py directly, we will have the self-import issue (not that it's a big issue)

but on the other hand we can't not import compute_rho_spahm in __init__, because it contains key functions we want to import for qstack.spahm.rho as a whole

2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def get_git_version_hash():
if not version.strip():
return VERSION + "+unknown"
print(version)
return VERSION+'+'+version.strip().decode()
return version.strip().decode()
Copy link
Contributor Author

@YAY-C YAY-C Dec 15, 2025

Choose a reason for hiding this comment

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

@briling @liam-o-marsh this is the smallest change I could find to make it work!
since the version is issued via github do we need to append the VERSION var?

Copy link
Contributor

@briling briling Dec 15, 2025

Choose a reason for hiding this comment

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

or we remove --tags

Copy link
Contributor

@briling briling Dec 15, 2025

Choose a reason for hiding this comment

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

the problem appeared when we added tag to the git but can we count they will be always here? @liam-o-marsh

Copy link
Contributor Author

@YAY-C YAY-C Dec 15, 2025

Choose a reason for hiding this comment

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

They have been around for quite some time ... I don't see why they would remove it.
Also, hard-encoding the version number into the script doesn't look very practical, and mainly prone to annoying mismatches. @briling What do you think?

Copy link
Contributor

@briling briling Dec 15, 2025

Choose a reason for hiding this comment

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

I mean if we remove them...
Hard coding is done for the case there's no git repo available i think
but I also think in this case it can be "unknown"

Copy link

@romaingrx romaingrx Dec 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nowadays I would say that we have everything in a pyproject.toml but otherwise we can read it from a __version__ variable in the package

Copy link
Contributor

@liam-o-marsh liam-o-marsh Dec 17, 2025

Choose a reason for hiding this comment

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

we would need to declare that __version__ manually in the __init__ file though, so that's still three places where the info needs to exist (package contents, pyproject.toml, git)

Copy link
Contributor

@liam-o-marsh liam-o-marsh Dec 17, 2025

Choose a reason for hiding this comment

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

numpy has a very complex build system so they can afford to dynamically generate a version.py file with the git hash too

Copy link
Contributor

@briling briling Dec 17, 2025

Choose a reason for hiding this comment

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

so what should we do? I think Yannick's changes work

Copy link
Contributor

@liam-o-marsh liam-o-marsh Dec 17, 2025

Choose a reason for hiding this comment

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

yeah they probably do



def check_for_openmp():
Expand Down
Loading