Refactor of LineList tables #409

profxj · 2017-09-06T18:46:00Z

This started as a branch to convert fk5 coords to icrs (fk5 only
uses float32 precision!) and became a refactor of LineList tables.

I also sped up some of the methods.

Note that I've turned off the 'extra' columns as the default
when initializing LineList but I can be convinced to have
this be on by default.

I imagine there will be some issues raised in pyigm and/or GUIs.

…tools/linetools into fk5_refactor # Conflicts: # linetools/scripts/lt_xabssys.py and a few more fk5 purges

jnburchett

There appear to be some recurring bugs dealing with calls to LineList._data, and I suspect these are symptomatic of a data table error that will rear its ugly head in several applications (e.g., instantiating AbsLines, AbsComponents, etc.)

jnburchett · 2017-09-06T19:36:57Z

linetools/abund/ions.py

@@ -37,6 +37,7 @@ def ion_name(ion, flg=0, nspace=None):
    -------

    """
+    import pdb; pdb.set_trace()


Why drop into the debugger here?

this method is now deprecated. I could just remove it I guess.

Just did.

jnburchett · 2017-09-06T19:59:01Z

linetools/analysis/utils.py

@@ -103,7 +103,7 @@ def gaussian_ew(spec, ltype, initial_guesses=None):
    # Use only good values (i.e. with meaningful errors)
    cond = (sig > 0.) & (np.isfinite(sig))
    # Actual fit
-    g = fit_g(g_init, wv[cond], fx[cond], weights=1./sig[cond])
+    g = fit_g(g_init, wv[cond].value, fx[cond].value, weights=1./sig[cond].value)


Line 65: I think this function is actually tested through test_gaussew_absline

It isn't passing astropy 2.0.1 without this change.

Sorry, my comment should have been inserted much higher and has nothing to do with your committed change. I was referring to your comment just below the docstring: "Note: Tested in test_absline_anly"

jnburchett · 2017-09-06T20:22:18Z

linetools/lists/linelist.py

@@ -593,48 +621,46 @@ def all_transitions(self, line):
        if line == 'unknown':
            return self.unknown_line()
        if self.list in ['H2']:
-
            cond = self._data['ion_name'] == line.split('(')[0]


Note that the self._data['ion_name'] does not exist. Should be self._data['name'].

jnburchett · 2017-09-06T20:30:49Z

linetools/lists/linelist.py

-                else:  # this should be always len(tbl)==1 because Z is not None
-                    return self.from_table_to_dict(tbl)
-            else:
+            indices = np.where(self._data['ion_name'] == line)[0]


See above also: self._data['ion_name'] does not exist; should be self._data['name']. Calling, e.g., LineList.all_transitions('HI') fails currently. However, it appears that self._data['ion_name'] is set in LineList. set_extra_columns_to_datatable()? Either way, all_transitions() doesn't work with or without 'extras'.

jnburchett · 2017-09-06T20:32:21Z

linetools/lists/linelist.py


-    def strongest_transitions(self, line, wvlims, n_max=3, verbose=False):
-        """ Find the strongest transition for an ion
+    def strongest_transitions(self, line, wvlims, n_max=3, verbose=False,


This method doesn't work either, also due to _data['ion_name'] KeyError.

see comment below

Works as expected now.

profxj · 2017-09-06T23:43:36Z

Several of the methods in LineList(), e.g. all_transitions
require that the set_extra_columns_to_datatable() has been run.

Previously, this was the default but I've turned that off. This will
give folks trouble, I suppose. And, as I think more carefully, I really
don't like adding these extra columns. Better to carry around a
separate table. Giving that a try now @jnburchett and @ntejos

coveralls · 2017-09-07T01:39:53Z

Changes Unknown when pulling 7a77e02 on fk5_refactor into ** on master**.

coveralls · 2017-09-07T02:38:15Z

Changes Unknown when pulling e7d3c9d on fk5_refactor into ** on master**.

jnburchett · 2017-09-07T22:25:59Z

This guy ready to look at again?

profxj · 2017-09-07T22:34:43Z

yes, please

jnburchett

Good news: available_transitions, all_transitions, etc., seem to be working well now that the table keys are straightened out.

I spot checked a few VP fits with joebvp, which accesses atomic data through linetools, and all seems to be well there.

Bad news: I tested igmguesses and some odd behavior is now occurring. For one, when I start up igmguesses, the gui shows an odd assortment of lines in the various panels (beginning with MgI and PbII). When I switch to the 'strong' linelist with the 'T' key, I then get the normal suite of lines I usually see starting with Ly-alpha. Things get weirder when I try to add a component, as the list of added components shows the wrong ion!

I suspect that @ntejos will be able to track down the source of the problem fairly easily. Lines 240-262 of pyigm.guis.igmguesses initialize the linelists to be used by igmguesses and 311 and 326-355 set the list for use.

jnburchett · 2017-09-07T23:00:22Z

linetools/lists/linelist.py


-    def strongest_transitions(self, line, wvlims, n_max=3, verbose=False):
-        """ Find the strongest transition for an ion
+    def strongest_transitions(self, line, wvlims, n_max=3, verbose=False,


Works as expected now.

jnburchett · 2017-09-07T23:34:51Z

linetools/analysis/utils.py

@@ -103,7 +103,7 @@ def gaussian_ew(spec, ltype, initial_guesses=None):
    # Use only good values (i.e. with meaningful errors)
    cond = (sig > 0.) & (np.isfinite(sig))
    # Actual fit
-    g = fit_g(g_init, wv[cond], fx[cond], weights=1./sig[cond])
+    g = fit_g(g_init, wv[cond].value, fx[cond].value, weights=1./sig[cond].value)


Sorry, my comment should have been inserted much higher and has nothing to do with your committed change. I was referring to your comment just below the docstring: "Note: Tested in test_absline_anly"

profxj · 2017-09-08T13:13:47Z

I am pretty sure the changes to igmguesses will be straightforward enough.
And I'm happy doing them. I just don't want to do so until @ntejos indicates
he is ok with the overall refactoring.

ntejos · 2017-09-08T13:35:31Z

I'm aware of this, but I may need some extra time before I can start looking closer (still traveling). Regarding igmguesses, we can leave that for the very end and I agree changes there will be relatively easy.

ntejos

Looks pretty good, specially the handling of available transitions and (hopefully) strongest transitions, if my "head compilation" was done right the later may need extra work to ensure the original sortening. Left some comments for your consideration...

ntejos · 2017-09-11T16:27:26Z

linetools/isgm/abscomponent.py

@@ -359,12 +359,15 @@ def add_abslines_from_linelist(self, llist='ISM', init_name=None, wvlim=None, mi

        # unify output to be always QTable


update inline comment QTable -> Table

ntejos · 2017-09-11T16:30:44Z

linetools/lists/linelist.py

@@ -65,7 +67,7 @@ class LineList(object):
    sort_by : str or list of str, optional


remove use_ISM_table from docstring as it is removed below from code.

checked that the docs (linelist.rst) is up to date too

ntejos · 2017-09-11T16:33:39Z

linetools/lists/linelist.py

+        data_file = resource_filename('linetools', 'data/lines/linelist.ascii')
+        # Read
+        self._fulltable = Table.read(data_file, format='ascii.ecsv')
+


Thus, we are not making the table from scratch anymore?

ntejos · 2017-09-11T16:36:22Z

linetools/lists/linelist.py

@@ -247,21 +260,25 @@ def set_lines(self, verbose=True, use_cache=True, use_ISM_table=True):
        use_ISM_table : bool, optional
          For speed, use a saved ISM table instead of reading from original source files.
        """


remove use_ISM_table as it seems is not used anymore.

ntejos · 2017-09-11T16:43:33Z

linetools/lists/linelist.py

            return

        if self.list not in ['HI', 'ISM', 'EUV', 'Strong', 'H2']:
            warnings.warn('Not implemented: will not set relative strength for LineList: {}.'.format(self.list))
            return

        # Set ion_name column
+        ion_name = np.array([' '*20]*len(self.name)).astype(str)


This may be a good time to set up a naming convention that is valid for atoms and molecules alike? Instead of having different ones as currently.

well, I'm not sure we have a convention at all.
Only 2 different prescriptions to generate them.

It might be good to have one method that does them.
But I think we are ok for now.

ntejos · 2017-09-12T00:52:07Z

linetools/lists/parse.py

@@ -912,51 +933,123 @@ def update_wrest(table, verbose=True):
    table['Ek'][mt[0]] = 52330.33 / u.cm
    '''
 #
+def load_datasets(datasets, tol=1e-3, use_cache=True):


make tol lower? 1e-4?

would require quite a bit of fussing.
Let's stick with 1e-3 for now.

ntejos · 2017-09-12T00:52:36Z

linetools/lists/parse.py


+    Parameters
+    ----------
+    use_ISM_table : bool, optional


remove use_ISM_table from docstring

ntejos · 2017-09-12T00:53:06Z

linetools/lists/parse.py


+    Parameters


define datasets in docstring

ntejos · 2017-09-12T00:54:12Z

linetools/lists/tests/test_init_linelist.py

 def test_ism_read_source_catalogues():
-    ism = LineList('ISM', use_ISM_table=False)
+    ism = LineList('ISM')#, use_ISM_table=False)


remove for good

ntejos · 2017-09-12T00:56:01Z

linetools/lists/tests/test_init_linelist.py

+    #with pytest.raises(ValueError) as tmp:  # This is failing Python 2.7 for reasons unbenknownst to me
+    #    ism.set_extra_columns_to_datatable(abundance_type='incorrect_one')
+    #ism = LineList('ISM')
+    #with pytest.raises(ValueError):


How do we test for expected errors now?

I fussed with this for quite awhile. I can't
get it to work on 2.7 (it is fine on 3.6).

…tools/linetools into fk5_refactor # Conflicts: # linetools/isgm/abssystem.py # linetools/scripts/lt_xabssys.py

profxj · 2017-09-12T16:21:10Z

@ntejos , @jnburchett -- I think we are there (but let's see if this
passes Travis).

Am going to fuss with igmguesses to make sure that works.

coveralls · 2017-09-12T17:00:33Z

Changes Unknown when pulling fe60a20 on fk5_refactor into ** on master**.

coveralls · 2017-09-12T17:21:50Z

Changes Unknown when pulling fe60a20 on fk5_refactor into ** on master**.

profxj · 2017-09-13T11:56:02Z

@jnburchett -- if you are happy and you know it, approve this PR.

Thanks.

coveralls · 2017-09-13T13:12:14Z

Changes Unknown when pulling bf9d89e on fk5_refactor into ** on master**.

jnburchett · 2017-09-15T16:35:44Z

Tested IGMguesses and problems seem to have been fixed.

profxj added 20 commits August 1, 2017 11:32

convert fk5 -> icrs

fc5870f

am xabssys warning too

2cd117c

Merge branches 'fk5_refactor' and 'master' of https://github.com/line…

56fd16c

…tools/linetools into fk5_refactor # Conflicts: # linetools/scripts/lt_xabssys.py and a few more fk5 purges

a few more fk5 purges

9944216

astropy 2.0.1

73b0414

fussing..

a140db3

tejos code..

e09af09

passing on my machine

d1ef307

still suffering.. somehow aux columns change!

f8bfd8f

progressing (I hope)

27c8ff5

struggling with masked table..

ee1e9b8

linelist made

5487b92

sped up available

a81d163

refactor is there

866109c

passing 2.0.1

a1e8ba0

fussing with travis

c251659

whole new travis file

7ce8666

speclite file

4436751

reducing the tests

0c7abc8

fix 2.7

7a77e02

profxj requested a review from ntejos September 6, 2017 18:46

profxj assigned jnburchett Sep 6, 2017

jnburchett requested changes Sep 6, 2017

View reviewed changes

moved extras to self._extra_table

e7d3c9d

jnburchett requested changes Sep 7, 2017

View reviewed changes

ntejos requested changes Sep 12, 2017

View reviewed changes

profxj added 2 commits September 12, 2017 09:20

Merge branches 'fk5_refactor' and 'master' of https://github.com/line…

91e3782

…tools/linetools into fk5_refactor # Conflicts: # linetools/isgm/abssystem.py # linetools/scripts/lt_xabssys.py

edits to NT

fe60a20

ntejos approved these changes Sep 12, 2017

View reviewed changes

sorting in subset

bf9d89e

jnburchett approved these changes Sep 15, 2017

View reviewed changes

jnburchett merged commit 21556df into master Sep 15, 2017

profxj deleted the fk5_refactor branch September 15, 2017 18:04

This was referenced Sep 28, 2017

Use ICRS instead of fk5 in coordinate names, etc. #405

Closed

Need to update the ISM LineList FITS file #265

Closed

		@@ -359,12 +359,15 @@ def add_abslines_from_linelist(self, llist='ISM', init_name=None, wvlim=None, mi

		# unify output to be always QTable

		@@ -65,7 +67,7 @@ class LineList(object):
		sort_by : str or list of str, optional

Refactor of LineList tables #409

Refactor of LineList tables #409

Conversation

profxj commented Sep 6, 2017

jnburchett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profxj commented Sep 6, 2017 • edited

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

jnburchett commented Sep 7, 2017

profxj commented Sep 7, 2017

jnburchett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profxj commented Sep 8, 2017

ntejos commented Sep 8, 2017

ntejos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profxj commented Sep 12, 2017

coveralls commented Sep 12, 2017

coveralls commented Sep 12, 2017

profxj commented Sep 13, 2017

coveralls commented Sep 13, 2017

jnburchett commented Sep 15, 2017

profxj commented Sep 6, 2017 •

edited