Read joeb VP output file #418

profxj · 2017-10-09T18:08:24Z

As titled.

Returns a list of AbsComponents but @jnburchett or @ntejos
may want more.

Test but no docs.

jnburchett

Our separate codes to accomplish the same task are quite different in some important ways. I've developed my code while dealing with the huge, and at times challenging, CASBaH lists. For one, I split up the step from joebvp to AbsLines and AbsLines to AbsComponents, as I think that may be useful for other applications. Perhaps we should iterate on this a bit? Your code is definitely cleaner, so the right answer is probably in between.

jnburchett · 2017-10-12T18:50:16Z

linetools/isgm/io.py

@@ -41,3 +50,67 @@ def abssys_from_json(filename):

    # Return
    return abs_sys
+
+
+def read_joebvp(filename, coord, llist=None):


Should add 'specfile' argument. Will explain below.

added. optional for now.

jnburchett · 2017-10-12T19:00:40Z

linetools/isgm/io.py

+    nflags, nidx = np.unique(vp_data['nflag'], return_index=True)
+
+    # Subset by nflag; Build components
+    for nflag in nflags:


I think we shouldn't pick out components by the 'nflag' values. I see the logic and while this will work in many cases, 'nflag' can also take on values 0 (let parameter vary without being tied to other lines) or 1 (fix parameter value). My approach chooses components based on 'zsys' and then 'trans' (which should probably be called 'species', but anyway...). However, this isn't perfect either, because one may already have declared multiple components with the same systemic redshift but different velocities during the fit. The best approach might involve employing the clustering analysis to group different transitions of the same species and redshift by velocity.

egads, I worry about the clustering.

Let me try zsys + trans.

jnburchett · 2017-10-12T19:06:22Z

linetools/isgm/io.py

+    for nflag in nflags:
+        mt_lines = np.where(vp_data['nflag'] == nflag)[0]
+        if len(np.unique(vp_data['vel'][mt_lines])) != 1:
+            pdb.set_trace()


This is immediately tripped by the CASBaH parameter file I tried loading because we will occasionally let the velocity centroid of a line flop around a little in the fit due to the COS wavelength uncertainty. Another reason the clustering analysis is perhaps useful here.

gack. am still resisting to the clustering.
as a human, we had every intent of grouping
these lines together as a component, right?

anyways, have made this an optional check and
defaulted to False

jnburchett · 2017-10-12T19:24:55Z

linetools/isgm/io.py

+            zlim = [vp_data['zsys'][idx] +
+                    vp_data[vkey][idx] * (1 + vp_data['zsys'][idx]) / ckms
+                    for vkey in ['vlim1', 'vlim2']]
+            absline = AbsLine(vp_data['restwave'][idx] * u.AA, z=z_fit, zlim=zlim, linelist=llist)


You could just declare the limits in velocity space too, right?

I don't think so. Not for a SpectralLine
But we could add that sometime.

jnburchett · 2017-10-12T19:28:54Z

linetools/isgm/io.py

+            absline.attrib['sig_b'] = vp_data['sigbval'][idx]
+            absline.attrib['z'] = z_fit
+            absline.attrib['sig_z'] = (1 + vp_data['z_comp'][idx]) * vp_data['sigvel'][idx] / ckms
+            absline.attrib['specfile'] = vp_data['specfile'][idx]


What's in the 'specfile' is probably not going to be especially useful, since it records the path to file at the time of the fit. What is useful (and even necessary for some of my analysis) is setting absline.analy['spec']. For this reason, the function should allow the user to specify the spectrum, either an XSpectrum1D object or a path to the file.

jnburchett · 2017-10-12T19:31:09Z

linetools/isgm/io.py

+            absline.attrib['coord'] = coord
+            absline.attrib['flag_N'] = 1
+            absline.attrib['logN'] = vp_data['col'][idx]
+            absline.attrib['sig_logN'] = vp_data['sigcol'][idx]


absline.attrib['N'] and 'sig_N' should also be set, as these are needed for AbsComponent.synthesize_column()

…etools/linetools into ingest_joebvp

ntejos

Please rename method and perhaps moving it to isgm.utils? otherwise move joebvp_from_components() to isgm.io.

Please add more information to docstring and, if possible, also to the .rst documentation.

ntejos · 2017-10-19T19:13:24Z

linetools/isgm/io.py

+            setattr(abscomp, key, abscomp.attrib[key])
+        # Errors must be in first line!
+        assert abscomp.sig_logN > 0.
+        comps.append(abscomp)


"reliability" and "comment" columns should also be included.

I'm not sure how to do this. Going to leave for @ntejos

ntejos · 2017-10-19T19:14:22Z

linetools/isgm/io.py

+
+
+def read_joebvp(filename, coord, llist=None, specfile=None, chk_vel=False):
+    """


Wasn't a version of this already coded in package joebvp?

I don't know. @jnburchett ?

ntejos · 2017-10-19T19:15:37Z

linetools/isgm/io.py

+    ----------
+    filename : str
+      joeB VP filename
+    coord : SkyCoord


Explain coord is supposed to be the coordinate of the QSO sightline for the AbsComponents...

ntejos · 2017-10-19T19:16:24Z

linetools/isgm/io.py

+    filename : str
+      joeB VP filename
+    coord : SkyCoord
+    llist : LineList, optional


Explain how llist will be used if given, and what happens otherwise.

ntejos · 2017-10-19T19:16:51Z

linetools/isgm/io.py

+      joeB VP filename
+    coord : SkyCoord
+    llist : LineList, optional
+    specfile : str, optional


Explain specfile is the name of the joebvp file.

ntejos · 2017-10-19T19:18:41Z

linetools/isgm/io.py

+        if chk_vel:
+            if len(np.unique(vp_data['vel'][mt_lines])) != 1:
+                pdb.set_trace()
+        z_fit = vp_data['zsys'][mt_lines[0]] + vp_data['vel'][mt_lines[0]] * (1 + vp_data['zsys'][mt_lines[0]]) / ckms


use ltu.z_from_v() instead.

ntejos · 2017-10-19T19:23:31Z

linetools/isgm/io.py

+            absline.attrib['z'] = z_fit
+            absline.attrib['sig_z'] = (1 + vp_data['z_comp'][idx]) * vp_data['sigvel'][idx] / ckms
+            if specfile is None:
+                absline.attrib['specfile'] = vp_data['specfile'][idx]


Is this something we want? If the joebvp file already has the specfile, why should we allow the user to change it?

@jnburchett had a good reason (see above)

ntejos · 2017-10-19T19:25:07Z

linetools/isgm/tests/files/group_9.VP

@@ -0,0 +1,4 @@
+specfile|restwave|zsys|col|sigcol|bval|sigbval|vel|sigvel|nflag|bflag|vflag|vlim1|vlim2|wobs1|wobs2|pix1|pix2|z_comp|trans 


Optional columns "comment" and "reliability" may exist.

ntejos · 2017-10-19T19:30:39Z

linetools/isgm/io.py

@@ -41,3 +51,83 @@ def abssys_from_json(filename):

    # Return
    return abs_sys
+
+
+def read_joebvp(filename, coord, llist=None, specfile=None, chk_vel=False):


Name should be something like: components_from_joebvp() (Note that we already have isgm.utils.joebvp_from_components(). Maybe worth using both in the test case.

renamed a few things as suggested

…etools/linetools into ingest_joebvp

coveralls · 2017-10-23T12:59:02Z

Coverage increased (+0.1%) to 71.931% when pulling 07fac5c on ingest_joebvp into dd5c4a8 on master.

ntejos · 2017-10-23T14:14:37Z

Have added a commit for reliability and comments. Please review. Also, it seems that "z_fit" is the same as "z_comp". If this is the case, would be good to leave only one of them in the code.

profxj · 2017-10-23T22:28:18Z

Merging

coveralls · 2017-10-23T22:31:26Z

Coverage increased (+0.1%) to 71.926% when pulling 5ca6851 on ingest_joebvp into dd5c4a8 on master.

read joevp output file

15eaf18

profxj requested a review from jnburchett October 9, 2017 18:08

profxj assigned marijana777 Oct 9, 2017

jnburchett requested changes Oct 12, 2017

View reviewed changes

Merge branches 'ingest_joebvp' and 'master' of https://github.com/lin…

940c733

…etools/linetools into ingest_joebvp

profxj requested a review from ntejos October 19, 2017 11:38

fixes to JB

5a53ad5

ntejos requested changes Oct 19, 2017

View reviewed changes

profxj added 4 commits October 23, 2017 04:20

Merge branches 'ingest_joebvp' and 'master' of https://github.com/lin…

0ca8e89

…etools/linetools into ingest_joebvp

corrections to NT

a645611

tests and docs

b28641e

add VP

07fac5c

comment, reliability and sig_z

8c971ec

fixing test

5ca6851

profxj merged commit 7fbc656 into master Oct 23, 2017

profxj deleted the ingest_joebvp branch October 23, 2017 22:28



		def read_joebvp(filename, coord, llist=None, specfile=None, chk_vel=False):
		"""

		@@ -0,0 +1,4 @@
		specfile\|restwave\|zsys\|col\|sigcol\|bval\|sigbval\|vel\|sigvel\|nflag\|bflag\|vflag\|vlim1\|vlim2\|wobs1\|wobs2\|pix1\|pix2\|z_comp\|trans

Read joeb VP output file #418

Read joeb VP output file #418

Conversation

profxj commented Oct 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

coveralls commented Oct 23, 2017

ntejos commented Oct 23, 2017

profxj commented Oct 23, 2017

coveralls commented Oct 23, 2017