AbsComponent work #414

profxj · 2017-10-01T22:55:06Z

Started as a simple bug fix but worked into a
refactor of table_from_complist() and its opposite.

Modified docs, tests, and NB

coveralls · 2017-10-01T23:11:58Z

Coverage increased (+0.04%) to 71.785% when pulling e3e6bbd on uniq_bug_fix into 0df35cb on master.

coveralls · 2017-10-03T01:27:54Z

Coverage increased (+0.04%) to 71.785% when pulling a6cbc6a on uniq_bug_fix into 0df35cb on master.

ntejos

Some comments for revision.

ntejos · 2017-10-03T17:20:38Z

docs/abscomp.rst

@@ -64,6 +64,11 @@ from the hard-drive::

 ::::

+One may also generate a set of components from a larger


Is it a set or a list?

list; fixed

ntejos · 2017-10-03T17:21:32Z

docs/abscomp.rst

-(mandatory column names include: `['RA', 'DEC', 'ion_name', 'z_comp', 'vmin', 'vmax']`)::
+You can also create a list of components using an input `astropy.table.Table` object
+(mandatory column names are
+`['RA', 'DEC', 'ion_name', 'z_comp', 'vmin', 'vmax', 'Z', 'ion', 'Ej']`)::


Is ion_name mandatory?

ntejos · 2017-10-03T17:22:51Z

docs/abscomp.rst

-to each corresponding AbsComponent in the `complist`. In this example, only those AbsLines expected to have rest-frame
-equivalent widths larger than `0.01*u.AA` will be appended, but if you wish to include all of the available AbsLines
-you can set `min_Wr=None`. Here, we have set `chk_sep=False` to avoid checking for coordinates because by construction the
+This will look for transitions in the `LineList('ISM')` object and append those within


remove "within" in this line

ntejos · 2017-10-03T17:26:08Z

docs/abscomp.rst

   tab['ion_name'] = ['HI', 'HI', 'CIV', 'SiII', 'OVI']
+   tab['Z'] = [1,1,4,14,8]
+   tab['ion'] = [1,1,4,2,6]
   tab['z_comp'] = [0.05, 0.0999, 0.1, 0.1001, 0.3]


ntejos · 2017-10-03T17:29:04Z

linetools/isgm/utils.py

@@ -270,7 +271,7 @@ def complist_from_table(table):

    Notes
    -----
-    Mandatory column names: 'RA', 'DEC', 'ion_name', 'z_comp', 'vmin', 'vmax'
+    Mandatory column names: 'RA', 'DEC', 'ion_name', 'z', 'vmin', 'vmax'


should be z_comp, to differenciate from Z.

ntejos · 2017-10-03T17:35:36Z

linetools/isgm/utils.py

+            else:
+                if table[key].unit is not None:
+                    abscomp.attrib[key] *= table[key].unit
+        complist.append(abscomp)


Or check all units properly afterwards here... but cannot have column with wrong units in the output AbsComponent.

Not sure I follow this one.

ntejos · 2017-10-03T17:39:52Z

linetools/isgm/utils.py

@@ -360,29 +347,61 @@ def table_from_complist(complist):
    Special columns: 'name', 'comment', 'logN', 'sig_logN', 'flag_logN'
    See also complist_from_table()
    """
+    key_order = ['RA', 'DEC', 'name', 'z_comp', 'sig_z', 'Z', 'ion', 'Ej',
+                 'vmin', 'vmax','ion_name', 'flag_N', 'logN', 'sig_logN',
+                 'b','sig_b', 'specfile']


reliability should be included

It is introduced later.

Fine. Please update the docstring accordingly.

ntejos · 2017-10-03T17:40:47Z

linetools/isgm/utils.py

+    tab['ion'] = [icomp.Zion[1] for icomp in complist]
+
+    # . attributes
+    for attrib in ['zcomp', 'Ej', 'flag_N', 'logN', 'sig_logN']:


add reliability and comment here?

noted that these are required.

ntejos · 2017-10-03T17:41:03Z

linetools/isgm/utils.py

-    tab['name'] = [comp.name for comp in complist]
-    tab['reliability'] = [comp.reliability for comp in complist]
+    for key in ['comment', 'reliability']:
+        if hasattr(complist[0], key):


instead of being separated?

comment and reliability are not required
so I prefer to separate them.

ntejos · 2017-10-03T17:41:52Z

linetools/isgm/utils.py

@@ -407,6 +426,7 @@ def iontable_from_components(components, ztbl=None, NHI_obj=None):
    -------
    iontbl : Table
    """
+    warnings.warn("It is likely this method will be Deprecated", DeprecationWarning)


Fine by me, do not think I have used this much.

…tools/linetools into uniq_bug_fix

coveralls · 2017-10-05T13:14:02Z

Coverage increased (+0.05%) to 71.803% when pulling 323710e on uniq_bug_fix into 9ff0b74 on master.

coveralls · 2017-10-05T13:40:20Z

Coverage increased (+0.05%) to 71.803% when pulling 323710e on uniq_bug_fix into 9ff0b74 on master.

coveralls · 2017-10-06T01:41:16Z

Coverage increased (+0.05%) to 71.803% when pulling f1b0daf on uniq_bug_fix into 9ff0b74 on master.

ntejos · 2017-10-10T15:27:37Z

I'm still not fully convinced with the units thing... I feel is important to have that check done.

profxj · 2017-10-10T18:17:38Z

I'm willing to have a warning thrown. But
that is far as I'll go. Compromise @ntejos ?

ntejos · 2017-10-10T18:23:45Z

Sure, a warning should do. Fine to merge after the warning is implemented.

profxj · 2017-10-10T20:40:03Z

done. merging

profxj added 4 commits September 30, 2017 10:37

refactor comp/table methods

0f7a3e1

tests ok

11688a5

docs, Notebook, and CACHE

2d7fbe2

CACHE me

e3e6bbd

profxj requested a review from ntejos October 1, 2017 22:55

small edit for NB

a6cbc6a

ntejos reviewed Oct 3, 2017

View reviewed changes

profxj added 2 commits October 5, 2017 05:43

edits to NT

a53f4ff

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

323710e

…tools/linetools into uniq_bug_fix

more edits to NT

f1b0daf

units

6a44a1f

profxj merged commit 211ffff into master Oct 10, 2017

profxj deleted the uniq_bug_fix branch October 10, 2017 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AbsComponent work #414

AbsComponent work #414

profxj commented Oct 1, 2017

coveralls commented Oct 1, 2017

coveralls commented Oct 3, 2017

ntejos left a comment

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 5, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

ntejos Oct 5, 2017

ntejos Oct 3, 2017

profxj Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 6, 2017

ntejos commented Oct 10, 2017

profxj commented Oct 10, 2017

ntejos commented Oct 10, 2017

profxj commented Oct 10, 2017

		@@ -64,6 +64,11 @@ from the hard-drive::

		::::

		One may also generate a set of components from a larger

AbsComponent work #414

AbsComponent work #414

Conversation

profxj commented Oct 1, 2017

coveralls commented Oct 1, 2017

coveralls commented Oct 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 6, 2017

ntejos commented Oct 10, 2017

profxj commented Oct 10, 2017

ntejos commented Oct 10, 2017

profxj commented Oct 10, 2017