Fitter API and make test cases work

[drvdputt]

Here next step for #280. Aside from the change in the features table introduced by #283, the user-facing Model things are basically unchanged (unless users were "digging for the astropy model" to do certain things).

For deep debugging or testing, one can still get direct access to the astropy stuff via model.fitter.model (model is a Model, model.fitter is an APFitter, model.fitter.model is the underlying astropy model object).

You can look at the implementation details of APFitter to get a feel for how it works. But it is more important to take a good look at Fitter, and how its functions are used in Model.

I also addressed the issues with the test cases (avoiding the use of .loc), and they seem to be running smoothly.

Besides complete removal of base.py, a reference to PAHFITBase in the documentation was also removed.

After this, only one more big change is needed: PowerGaussian and PowerDrude, and the change from amplitude to power units for the Features table.

[jdtsmith]

I'm not sure exactly how to proceed, I had hoped many of your improvements would be separable from the new Model/Fitter setup, but I now see that's not the case. Maybe the right approach is to clean up some of the names and discrepancies (e.g. now we call wavelength w, x, xz, wav), but keep the structure basically as-is. Merge to dev, test for a while, the if all's OK release on master as v2.6. BUT, keeping the understanding that the Model <-> Fitter interface will probably have to change, which will require changes in APFitter to suit.

In terms of user-facing things we probably won't want to change ahead:

1. Do we really "leak" instrument information into the Features tables? I say only for the special case of a user having put in an unmasked FWHM ahead of time do we do this, otherwise, leave masked. This we call "leaving Features unsullied". If so, Model will have to keep track of the "sullied fit results" somehow separately. Right now there are a lot of expectations of sullied Features (e.g. in Model.plot). plot can just add masked fwhm's as needed at the last moment, in fact should do so.
2. Are we certain we want users to mymodel.features.loc['h2_s01']['power'] vs. just mymodel.loc['h2_s01']['power']. I.e. isn't Model mostly just a Features table with some functionality for setting up a Fitter? We could easily keep the same system and provide indexing pass-through wrappers on Model. So maybe that doesn't have to be decided now.

[sarduval]

Lines 183-186 of model.py:

        inst, z = self._parse_instrument_and_redshift(spec, redshift)
        # save these as part of the model (will be written to disk too)
        self.features.meta["redshift"] = inst
        self.features.meta["instrument"] = z

Is this correct? It seems that these definitions for redshift and instrument are flipped?

[drvdputt]

Lines 183-186 of model.py:

        inst, z = self._parse_instrument_and_redshift(spec, redshift)
        # save these as part of the model (will be written to disk too)
        self.features.meta["redshift"] = inst
        self.features.meta["instrument"] = z

Is this correct? It seems that these definitions for redshift and instrument are flipped?

This has now been fixed (see latest commits pushed).

I have also gone through most of the other comments. So we should be close to merging this, and then starting a new pull request to integrate my implementation of the power-features.

A few things revealed by the tests:

• @jdtsmith Linkcheck says the link to your website is broken (this one: http://tir.astro.utoledo.edu/jdsmith/research/pahfit.php)
• Codecov is complaining about needing a token. Not super important right now. I think we should fix all our continous integration services in a single pull request. Will take another look at the reports and summarize in an issue.
• I did a second push to fix a double space somewhere.

[jdtsmith]

Thanks for getting back into it Dries. My server has been cutoff at the firewall, working to restore access. I agree about the CI. Do we need to solve #294 now too given all the redshift discussion?

[jdtsmith]

Update: my server will unfortunately not be allowed back on the external internet (new VPN-only policy); I've uploaded "PAHFIT classic" to https://github.com/PAHFIT/pahfit_classic. Can you update the internal links to point there?

Aside from the redshift discussion which seems to have wound down, what else is needed on this PR?

[drvdputt]

In addition to your website, there is an additional documentation issue I discovered. So I just pushed two new link fixes.

The biggest issue still remaining is cases like this:

            for row_index in np.where(self.features["kind"] == "line")[0]:
                row = self.features[row_index]
                if np.ma.is_masked(row["fwhm"]):

As in the comments above, row["fwhm"] can be a single element, or a tuple (so 0D or 1D). I ran into this issue while running one of my demo notebooks. There is no "readable" check that works for both. But I have a hacky workaround, which replaces some of of the np.ma.is_masked checks with .ndim == 0.

[jdtsmith]

But I have a hacky workaround, which replaces some of of the np.ma.is_masked checks with .ndim == 0.

Did you see my row["fwhm"] is ma.masked method above? That seems to be clearest and works for scalars and 1D things equally.

[drvdputt]

Looks promising doing some tests in the python shell. I will give it a shot and see if I can get my notebook to run.

[drvdputt]

Seems to work fine for now. But at one point, this logic will have to be revised, when we set up a clear way to allow users to fit the FWHM of specific lines. Will be addressed by #293.

Let's leave the redshift issue as is for now, so we can finish the work on the power features too. Once this has been merged, I can rebase my power features branch, and spin up the next pull request.

[jdtsmith]

OK, great, shall we go ahead and merge this to dev?

[drvdputt]

I think this is good to go. I will be actively using the dev branch (with my power features applied), so I will see if anything pops up along the way.