Skip to content

Conversation

@eteq
Copy link
Member

@eteq eteq commented Aug 15, 2011

This pile of commits define the setup.py and initial layout for the package.

One item I wasn't sure about: versioning. As it stands right now, version numbers are set by the "version" module, which creates version numbers of the form "x.x.x" if the release variable is True, and otherwise produces "x.x.xdev-r##" where ## is the total number of commits in the github repository. This requires a little trick to get the version numbers to properly freeze when installing a development version (see the astropy_build_ext in setup.py), but it works fine as-is.

I think it's better to at least have "dev" at the end of the version name - pypi, setupytools, and distutils all behave properly when you do this - a version that ends in the string "dev" is always an "earlier" version than one with "dev", and the "-r##" means you always know if you have forgotten to re-run python setup.py install or the like. But I could change it to just "x.x.xdev" or "x.x.xalpha" or something like that without the version number, if that's deemed a bad idea.

Copy link
Member

Choose a reason for hiding this comment

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

Since I see that you have the version stuff in a separate module, I wonder if it would make sense to have:

from astropy.version import version as __version__ here

since the unofficial seems to have packagename.__version__ available?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest not putting anything in astropy/init.py just yet., as I kind of always figured astropy would be a "namespace package". But that's not something that's really been discussed formally yet.

@astrofrog
Copy link
Member

This pull request seems fine to me. I like the layout. I definitely agree with adding dev (not alpha) at the end of the version numbers, and I agree that the equivalent revision number would be useful (note we could also just use the 6 first characters of the SHA, but I agree it's less intuitive). The only comment I have is regarding making version available, but to be honest this can be added later.

@eteq
Copy link
Member Author

eteq commented Aug 15, 2011

I agree that adding the __version__ variable makes sense, so I just updated it to do so. I also updated .gitignore to ignore some of the files we definitely want to ignore (after almost adding a .pyc file...)

@perrygreenfield
Copy link
Member

I may want to have a couple in our branch look at this. Is it typical to have the C extensions that separated from the Python code? Usually they are tightly coupled with a particular module or package and I wonder if it makes sense to put things in two different places. What is the advantage of doing that?

@eteq
Copy link
Member Author

eteq commented Aug 16, 2011

The advantage of a separate /extern directory for c code is that you can put "un-altered" C libraries in there so that they don't need to be downloaded separately, but can be easily updated in their own directories when the upstream library is updated.

The /astropy/wrappers directory is for the wrappers for the /extern c libraries only - it's in /astropy because it's clearer to have the Cython/C code be where it actually ends up in terms of the python package layout, but remain separated out so that it's clear that that code is directly interfacing with the /extern C libraries.

The intent is that any Cython or C code that is not directly related to an external library gets placed in the particular module it is used in, exactly as you say. These two separated directories are only meant for pre-existing C-libraries that need to be wrapped.

@embray
Copy link
Member

embray commented Aug 17, 2011

This looks good to me as a starting template, other than my above comment about namespace packages. That's not really an actionable comment at this point though, as there has been no discussion about it. My point about namespace packages, however, is that if astropy is going to be a large platform, it may be desireable to break out at least parts of it into separate optional installables, in which case namespace packages are the way to go. I don't know if anyone else had this in mind though.

[And as an aside on namespace packages in general, I'm aware that there's been discussion in scikits about not using them anymore, but I think that's misguided, especially in anticipation of PEP 382/402, either of which will make them easier to deal with once accepted.]

@eteq
Copy link
Member Author

eteq commented Aug 17, 2011

Actually, the intent as specified in the vision and coding guidelines is specifically not to have astropy be a namespace package. The idea is that the astropy package contains all the core functionality, and everything else uses the "awastropy" or no top-level namespace (just the package name directly), until it's merged into the astropy core.

So this pull request is about the layout of the core package, and the astropy/project-template package is the one that is the template for affiliated packages. (it will reflect this design fairly closely so as to make merging easier, but will have slightly different names and include examples and such)

Once this layout is finalized, we will implement the affiliated package index, and it is those affiliated packages that will allow for the separate optional installable you have in mind.

While I see the advantages of the namespace package idea, they're a pain to use given the problems that PEP 382/402 were designed to fix. 382/402 are great, but we can't use them while still maintaing 2.x compatibility (they only apply to 3.2 and 3.3). So at least for the near term, this scheme is more straightforward.

@embray
Copy link
Member

embray commented Aug 17, 2011

I guess my problem is that it hasn't really been defined what make up the "core functionality" required for other Astronomy packages. By some definitions that could be a lot of stuff. But I'm fine with saying "no namespace packages" for now, and it can be worried about if "being too big" actually becomes a real problem.

@eteq
Copy link
Member Author

eteq commented Aug 17, 2011

You have a good point to keep in mind - we should definitely have this option on the radar for the future as we merge other affiliated packages in and be sure to at least allow for a change to a namespace package if "too big" becomes a problem. If nothing else, hopefully by then everyone will have moved to 3.x and we can safely use the 382/402 enhancements.

Copy link
Member

Choose a reason for hiding this comment

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

I think /extern should be /cextern now

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch! Corrected.

@astrofrog
Copy link
Member

This looks good to me. Obviously the layout might still change slightly in future, but I think we should merge this for now.

eteq added a commit that referenced this pull request Sep 13, 2011
@eteq eteq merged commit 5360fcc into astropy:master Sep 13, 2011
astrofrog added a commit that referenced this pull request Oct 21, 2011
force freezing version to use log and skip odict when builtin in use
embray referenced this pull request in embray/astropy Dec 6, 2012
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
force freezing version to use log and skip odict when builtin in use
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
mwcraig referenced this pull request in mwcraig/astropy Aug 12, 2014
Use MSVC to compile instead of mingw
kelle pushed a commit to kelle/astropy that referenced this pull request Mar 24, 2016
nstarman referenced this pull request in nstarman/astropy Aug 31, 2020
Co-authored-by: Adrian Price-Whelan <[email protected]>
jeffjennings pushed a commit to jeffjennings/astropy that referenced this pull request Jul 2, 2025
Add PyVO as astropy affiliated package to registry.json
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.

4 participants