Moves msgs into pypit/__init__.py

[kbwestfall]

This PR moves the msgs object in the pypit/__init__.py file. For most files, the only change was to use from pypit import msgs instead of

from pypit import armsgs
msgs = armsgs.get_logger()

But I also had to make a number of changes to the main PYPIT function and the armsgs.Messages class to get this to work. A bit of a headache was to allow for the armsgs.Messages object to close the QA files. Forcing all the imports to the top of the file led to a circular import. What I have in place works, but I had to change the import from pypit.arqa import get_dimen, set_qa_filename in pypit/arpca.py to from pypit import arqa; otherwise, I was getting an error. This probably means that you always have to use this format; i.e., you can't call from pypit.arqa import ... without getting an error, which is not ideal.

So I think the larger question is do we really need armsgs.Messages.close to access and run the consolidated function arqa.close_qa? As far as I can tell, the function isn't closing already open QA files. Is it critical that these QA functions be run when an error is called and/or someone pressed 'ctrl+c'?

I also made some style changes to enforce PEP 8 in the files that I touched.

[kbwestfall]

It looks like there are some bugs here I need to fix in the tests. Let me try to figure this out before you review it.

[kbwestfall]

The travis errors are when the setup.py file is trying to import something from within pypit and it ends up creating a circular import (I think). I know virtually nothing about setup.py files, but is this a good idea? I.e., do we want the setup.py file to import anything from pypit? I'm not sure what the best practice is here.

[rcooke-ast]

Good idea to move the version check into setup.py, that's what I would have suggested first... The only problem is that archeck.py is also called from within pypit.py to check that the user has the correct versions before attempting to reduce their data. So, we either need to do the version checking only during setup (which seems sensible), or find a way around the circular import (hmmm...), or keep two distinct version_check() routines in the code (which I think is a bad idea...).

The current error seems to be that you're not importing scipy at the top of the file. Perhaps, in setup.py, try to import the same dependencies that are listed at the top of the archeck.py file. I also think we can probably comment out or delete the version checking that is performed in pypit.py for now. One way around this might be to have a single text file which lists the dependencies, and both setup.py and pypit.py can access this same text file. We would then need the same few lines of code to check the versions in setup.py and pypit.py. I don't see this as a big problem... What do you think?

[kbwestfall]

Do'h! Yeah, stupid bug on my part. I just added scipy for now to see if travis still fails (and it does...).

Right, one of things that I thought of suggesting was to clean up setup.py. I was putting this off because, like I said, I'm not really comfortable editing setup.py files yet, and I wanted to hold off, again, until all the cython is removed. My point of reference, again, is MaNGA's Marvin package:

https://github.com/sdss/marvin/blob/master/setup.py

There, they use a text file to set the requirements (requirements.txt). They don't check these versions themselves within the code; they become part of the installation process using the install_requires key in the setuptools.setup function. So we can have this one file that is both used to ensure that the packages are up-to-date when pypit is installed and also read whenever we want (at the beginning of the pypit function) to make sure that the dependencies are still being adhered to. I also don't like that the current setup has repeat code in setup.py and archeck.py, and I think this gets around that.

So maybe given that travis is failing, this change to the setup.py files is more pressing. Let me know what you think and I can try to fix this today/tomorrow.

[rcooke-ast]

This sounds good to me. PYPIT's setup.py file is currently based on the DESI pipeline (@profxj wrote this, maybe he can correct me if I'm mistaken?). In any case, if using a different style of setup.py file does the trick, I'm happy with that! I guess now's the time to do it, since we're also ditching cython.

[profxj]

I am fully supportive of over-hauling setup.py
As @rcooke-ast mentioned, it was a copy of something from DESI.

The current Travis failure is odd. I've added an explicit install
of arclines to the .travis file but that isn't working.. Am fussing
around further.

As for the requirements, I might prefer that they simply be checked
and not also installed when they aren't installed. I found the code
trying to install the new version of h5py for me and that failed.

[profxj]

Scanning the great internet, I don't see a way to check for
the dependencies without installing them using setup().

As such, we might want to fall back to using something like
archeck() although I agree that having setup.py import anything
from PYPIT is a bad idea..

[profxj]

Regarding 'close_qa', @kbwestfall is correct
that this doesn't do any closing. All it does is
generate the HTML files to enable viewing the
QA files that were generated. I should prefer
to keep this activity if possible.

[profxj]

ok, I've made chk_requirements a method in setup.py
and duplicated it in archeck. Not ideal, but I'm happy with
it. I also chose to move the requirements.txt file into pypit/
That might be required for archeck.py to be able to find it.

Now back to the arclines failures in Travis..

[kbwestfall]

Okay, there are a number of balls in the air so let me try to summarize:

• setup.py: I did rewrite this, and it seems to work fine (apart from some of the errors we're seeing in Travis). I think anything in the new requirements.txt file will be installed if it's not current. You can remove this dependency by just removing, or commenting with '#', the h5py line. We can add a version check back into setup.py if that's preferred, but I'd rather we stick the code into setup.py directly instead of it having to import something like archeck. Again, I don't know much about setup.py files (and I know the DESI one does this), but having setup.py import and use something from the package it's trying to install feels a bit wacky. Maybe it's not an issue, but how does python behave if the package is already installed? Will setup.py then be using the installed version, or the one that you're trying to install?

[kbwestfall]

• arclines (and linetools): I don't really understand why this wasn't causing Travis problems before, except for the following. I did move the import of arclines to the top of the file, so it might be that the specific test that imported arclines in the function itself wasn't being called. I talked to one of the Marvin developers about submodules because they use them in Marvin. Part of my e-mail and his response:

The reason I ask is because the PYPIT code base that I'm working with has some obvious submodule candidates, but so far they've been treated as different packages. I've started to impose some more modular design on the code, which causes Travis/pytest errors that for some reason weren't being tripped before. Specifically, one of the test functions requires the submodule, but the submodule isn't a formal "dependency" of the code so it isn't installed in the Travis build (the submodule isn't pip installable, and I don't yet know if there's a way around this). I want to try to avoid this test error by adding the package as a submodule, using marvin + sdss_access as an example.

We’re aiming to move some of our submodules out and make them separate dependencies (packages), e.g. sdss_access, tree, and marvin_brain. Submodules can be a bit tricky to maintain and keep the correct version updated in all places. I think a good rule is that if it’s something that can be released and used independently, you make it a separate pip-package. If it’s a small bit of code you need, and you want it as a separate repo, but doesn’t make much sense as a separate package, e.g. our version of sqlalchemy_boolean_search, keep it as a submodule. But it’s ultimately up to you.

Travis should be able to handle submodules that aren’t strict dependencies or pip-installable products. Travis will always check out the latest git repo and run a git submodule init and update. It’s up to you though to ensure they get loaded into your Python environment, which we do automatically upon the initial marvin import.

[kbwestfall]

• circular imports: I went ahead and edited the code to deal with a lot of the circular imports that Travis was running into with the python2 execution of the pytest suite. Most of this just involved moving many of the functions in arqa into the appropriate module. This means, though, that arqa is a very different module. I understand the idea of collecting all the plotting function into a single module, and this is something I would do in C/C++ all the time, but it's not as easy in python. So now, e.g., the trace QA plots are in artrace, etc. The only subtle thing I had to handle was that arextract.optimal_extract() was calling arproc.variance_frame(), which was causing a circular import. So now arextract.optimal_extract() returns the obj_model object, not the new variance frame. The new variance frame is calculated in arproc using the returned object from arextract.optimal_extract(). If this is confusing, we may need to rethink where (what module) to define variance_frame.

[profxj]

Here is a compromise on the QA/HTML:

The only bits I want done when the code crashes
is to regenerate the HTML. I'm fine with those few
methods being in an arhtml.py module which is not
allowed to import msgs. This avoids the circular imports.
I will try implementing this now.

I somewhat prefer all the QA methods being in one module
(i.e. arqa) but am not that wedded to it.

[profxj]

Ok, after spending my own time in circular import hell
I took out the from with arclines. I'll note that I also
took all of PYPIT out of arclines to avoid any circular
dependency there. That was a good thing but insufficient.

[profxj]

ok, passing tests now. Am going to
take a walk through the full PR soon.

[kbwestfall]

Awesome! Sorry for all the hassle. Hopefully it's a step in the right direction. A few thoughts:

• It might be worth tagging the existing master (or maybe the version before my first PR) to make it easy to go back to if we need it.
• If there is functionality that's common to both arclines and pypit (and linetools?) it might be worth consolidating it into a new (sub)package. We could call it pitcrew :)
• Once this gets merged, we probably want to make sure other developers pull this into their branches so that we can resolve any conflicts sooner rather than later.
• It may also be useful to clean house with the branches, particularly if we want to try to implement this new dev strategy.

[rcooke-ast]

This is looking much better - a huge step in the right direction!

I think it's worthwhile at this point either closing or discussing arclines Issue 26

Given that we're hoping to modularise PYPIT, perhaps it makes sense to merge arclines into PYPIT?

[profxj]

Ok, I've made my more formal pass.
We can discuss my comments tomorrow.

[profxj]

Add:

-- Updates archeck
-- Loads NIST arclines from arclines instead of PYPIT

[profxj]

Was this another circular import?
Just checking..

[kbwestfall]

No, not a circular import. Just a syntax preference. The module loads both scipy.stats and astropy.stats, so I just wanted to be clear where this comes from.

[profxj]

Is this a circular imports issue?

These changes would reduce the ease-of-use
of this debugger..

Let's discuss.

[kbwestfall]

I think I commented these out because they're not actually used in the module itself. Are you importing them here because they get used when debugger is imported?

[profxj]

yes, that is the intent.

[kbwestfall]

Ah, got it. I just saw how this is used in artrace.py . We can leave it that way if you want, but I'm not crazy about the construction. E.g., why are ginga.py and ardebug.py separate modules? In the one place chk_arc_tilts() is used (artrace.py) the module actually imports ginga as well (although that's probably because I moved all the imports to the top), so why not use it directly from ginga?

[profxj]

Ok, I think that makes good sense.

[profxj]

merging!