fix version-string (PEP compliant) and other stuff #134
Conversation
| return VERSION + "+unknown" | ||
| print(version) | ||
| return VERSION+'+'+version.strip().decode() | ||
| return version.strip().decode() |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
the problem appeared when we added tag to the git but can we count they will be always here? @liam-o-marsh
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
numpy has a very complex build system so they can afford to dynamically generate a version.py file with the git hash too
There was a problem hiding this comment.
so what should we do? I think Yannick's changes work
There was a problem hiding this comment.
yeah they probably do
YAY-C
changed the title
| @@ -1 +1,3 @@ | |||
| """Atom- and bond-based SPAHM module.""" | |||
|
|
|||
| from . import compute_rho_spahm | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, utilsI tried to check how you did it in the other folders, but it didn't seem systematic.
There was a problem hiding this comment.
oh I see now
do we need all the modules to be imported though?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
but I thought import qstack.spahm.rho imports __init__.py and the script is in __main__.py
There was a problem hiding this comment.
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
Closes #133
setup.pyqstack.spahm.rhoimport