Unique components #410

profxj · 2017-09-18T22:55:22Z

There are two threads to this PR:

Refactor from LineLimits to zLimits
Add a unique_components() method

The first allows for limits without reference to wrest.
That becomes optional, so previous functionality is maintained.

The second is a method useful for querying lists of components.

I also removed GaussianAbsorption1D as that is deprecated in astropy.

Includes a new test, but no new docs.

coveralls · 2017-09-18T23:13:57Z

Coverage increased (+0.1%) to 71.756% when pulling d1efe3d on unique_components into 21556df on master.

jnburchett

Few minor suggestions.

jnburchett · 2017-09-20T18:40:35Z

linetools/analysis/zlimits.py

@@ -23,7 +24,8 @@ class LineLimits(object):
    zlim : tuple of floats
      Redshift limits for a line
      Defined as wave/wrest - 1.
-    wvlim : Quantity array
+      or


Do not really need 'or' line, do we?

I even removed the line above as wrest is not required

jnburchett · 2017-09-20T18:43:36Z

linetools/analysis/zlimits.py

@@ -41,7 +43,7 @@ def from_specline(cls, aline, z, zlim):
        if not isinstance(aline, (AbsLine, EmLine)):
            raise IOError("Input aline must be AbsLine or EmLine")
        #
-        slf = cls(aline.wrest, z, zlim)
+        slf = cls(z, zlim, wrest=aline.wrest)


Need to add z and zlim to parameters list in the docstring.

jnburchett · 2017-09-20T18:45:54Z

linetools/analysis/zlimits.py

        z : float
          Redshift of the line.
        zlim : tuple or list
          Redshift limits for a line
          Defined as wave/wrest - 1.
          Ok to have zlim[1]==zlim[0], but then self.is_set() == False
+        wrest : Quantity, optional
+          Rest wavelength.  Used for SpectralLine objects


Doesn't have to be used by SpectralLine objects, right? I'm able to instantiate just by declaring wrest.

jnburchett · 2017-09-20T18:50:34Z

linetools/analysis/zlimits.py

@@ -142,7 +149,7 @@ def is_set(self):
            return False

    def set(self, inp, chk_z=False):#, itype='zlim'):
-        """ Over-ride = to re-init values
+        """ Set (or reset) limits relative to self._z


Maybe add a note that this does not change the reference redshift. Perhaps change to: "Set (or reset) limits relative to self._z but does not change self._z"

I was unclear as to what this did before I tried it.

agreed with the suggestion above.

jnburchett · 2017-09-20T22:58:24Z

linetools/isgm/abscomponent.py

-        self.vlim = vlim
+        # Limits
+        zlim = ltu.z_from_dv(vlim, zcomp)
+        self.limits = zLimits(zcomp, zlim.tolist())


I think there needs to be a way to set the reference redshift, e.g., mycomp.limits.setz(0.4)

One could imagine plenty of scenarios where one may wish to instantiate an AbsLine or AbsComponent with some lines then change the redshift on later. I realize this can be done by setting limits._z, but that's a bit under the hood and may not occur to a user who did not review the PR ;)

Good idea, should probably be a method of zLimit class.

I understand the desire, but how does one modify the limits
in that case? Leave them as is?

I think I'd argue that the user should in that case instantiate
a new zLimits object.

jnburchett · 2017-09-20T23:26:08Z

linetools/isgm/utils.py

    # Set vlim by min/max  [Using non-relativistic + buffer]
    vmin = u.Quantity([(comp.zcomp-zcomp)/(1+zcomp)*const.c.to('km/s')+comp.vlim[0] for comp in components])
    vmax = u.Quantity([(comp.zcomp-zcomp)/(1+zcomp)*const.c.to('km/s')+comp.vlim[1] for comp in components])
-    synth_comp.vlim = u.Quantity([np.min(vmin)-vbuff, np.max(vmax)+vbuff])
+    vlim = u.Quantity([np.min(vmin)-vbuff, np.max(vmax)+vbuff])


Looks like the default value of vbuff is 0 km/s but the docstring indicates 10 km/s?

Must have used to have been 10 km/s. I guess
I figured that was a problem..

Have edited the doc string accordingly and kept it at 0 km/s.

jnburchett · 2017-09-20T23:27:41Z

linetools/isgm/utils.py

+def unique_components(comps1, comps2, tol=5*u.arcsec):
+    """ Identify which members of comps1 are *not* within
+    comps2, to given tolerances.  Note, that repeats in
+    comps1 are not looked for.


Perhaps clarify 2nd sentence? "Note: nonunique members of comps1 are not identified."

I do not understand this either. How can a component be within another component? This is based on velocity limits or was supposed to identify two components that may be considered the same physical entity?

have tried to clarify.

This remains unclear to me. I beleive you try to identify component duplications (?), but in this case why to allow for a tolerance? If you allow a tolerance (specially in velocity limits), then what would happen for multiple contiguous and overlapping components? Will there be considered the same? If so, why is this useful?

Oops, I forgot to push utils.py

Look at the new docstring please

ntejos

Left some comments. I think is a good idea to add a method in zLimits to reset z. Probably with some restrictions like:

New z must be within zlims of the previous one
wvlims must be preserved (?)

Another option is to add a resetz method to AbsComponent, redefining the zLimits from scracth.

ntejos · 2017-09-21T00:30:18Z

linetools/analysis/zlimits.py

@@ -23,7 +24,8 @@ class LineLimits(object):
    zlim : tuple of floats
      Redshift limits for a line
      Defined as wave/wrest - 1.
-    wvlim : Quantity array
+      or
+    wvlim : Quantity array, only if _wrest is set
      wavelength limits for the line in observer frame


this docstring only if _wrest is set is confusing. Maybe say that this is an optional parameter that applies for zLimits to individual AbsLines (for which a single wrest makes sense)?

ntejos · 2017-09-21T00:31:08Z

linetools/analysis/zlimits.py

@@ -23,7 +24,8 @@ class LineLimits(object):
    zlim : tuple of floats


add z to docstring

ntejos · 2017-09-21T00:33:12Z

linetools/analysis/zlimits.py

        """
        Parameters
        ----------
-        wrest : Quantity
-          Rest wavelength of the line.
        z : float
          Redshift of the line.


redshift of the line -> redshift to apply the zlimits to

edited. ps. your version looks a bit out of date

ntejos · 2017-09-21T00:34:14Z

linetools/analysis/zlimits.py

        z : float
          Redshift of the line.
        zlim : tuple or list
          Redshift limits for a line
          Defined as wave/wrest - 1.
          Ok to have zlim[1]==zlim[0], but then self.is_set() == False
+        wrest : Quantity, optional
+          Rest wavelength.  Used for SpectralLine objects


Used for SpectralLine objects -> Used for applying zLimits to individual SpectralLine objects

ntejos · 2017-09-21T00:36:35Z

linetools/analysis/zlimits.py

@@ -142,7 +149,7 @@ def is_set(self):
            return False

    def set(self, inp, chk_z=False):#, itype='zlim'):
-        """ Over-ride = to re-init values
+        """ Set (or reset) limits relative to self._z


agreed with the suggestion above.

ntejos · 2017-09-21T00:39:19Z

linetools/isgm/abscomponent.py

-        self.vlim = vlim
+        # Limits
+        zlim = ltu.z_from_dv(vlim, zcomp)
+        self.limits = zLimits(zcomp, zlim.tolist())


Good idea, should probably be a method of zLimit class.

ntejos · 2017-09-21T00:44:33Z

linetools/isgm/utils.py

+def unique_components(comps1, comps2, tol=5*u.arcsec):
+    """ Identify which members of comps1 are *not* within
+    comps2, to given tolerances.  Note, that repeats in
+    comps1 are not looked for.


I do not understand this either. How can a component be within another component? This is based on velocity limits or was supposed to identify two components that may be considered the same physical entity?

profxj · 2017-09-21T14:17:29Z

Ok, the outstanding issue is how to modify the redshift if at all.

I am preferring that the user re-instantiate, i.e. self._z is fixed.
But you can convince me otherwise.

Another option would be a method that takes in a new redshift and
shifts the limits accordingly. It would also reinstantiate, e.g.

self.limits = self.limits.shift_z(z)

Thoughts?

ntejos · 2017-09-21T14:32:07Z

Now that I re-think about it, zLimits is a utility class to be used within the framework of SpectralLines, AbsComponents, etc. It is not meant for stand-alone use (let me know if you envision where this would be useful). In that context, it may be preferable (cleaner) that for changing the redshift, one has to create a new object from scratch. The advantage for the alternative is speed, but it is not obvious what to do with the limits when self._z changes. Thus, cleaner may be better than faster in this context.

coveralls · 2017-09-21T14:33:54Z

Coverage increased (+0.1%) to 71.756% when pulling baade8c on unique_components into 21556df on master.

coveralls · 2017-09-21T15:16:24Z

Coverage increased (+0.1%) to 71.756% when pulling 03a1cba on unique_components into 21556df on master.

jnburchett · 2017-09-21T15:38:59Z

Yeah, I'm not married to the idea of modifying the z of an existing zLimits object if you guys feel strongly that the user should create a new one. However, at the very least can we inform the user that they need to reinstantiate for a different redshift? As for what to do with the limits once we change zlim (if we did), one basic option is to keep the vlim and adjust zlim accordingly. I see that I'm able to change the z of an AbsLine with .setz(), so the point is moot in most applications I suppose. Here the user is advised to update limits but not required to. If I change the vlim after using setz(), the zlim are updated as well, so I suppose what I foresee doing is possible given current functionality.

…

-Joe

On Sep 21, 2017, at 7:50 AM, J. Xavier Prochaska ***@***.***> wrote: @profxj commented on this pull request. In linetools/isgm/utils.py: > @@ -886,3 +888,58 @@ def joebvp_from_components(comp_list, specfile, outfile): f.close() +def unique_components(comps1, comps2, tol=5*u.arcsec): + """ Identify which members of comps1 are *not* within + comps2, to given tolerances. Note, that repeats in + comps1 are not looked for. Oops, I forgot to push utils.py Look at the new docstring please — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

jnburchett · 2017-09-21T15:47:00Z

linetools/isgm/utils.py

-    comps1 are not looked for.
+    """ Identify which AbsComponent members of the comps1 list
+    are *not* within the comps2 list, to given tolerances.
+    Note, AbsComponent objects in the comps1 list are examined


AbsComponent objects in the comps1 list are not examined against each other right? I though that's what the previous docstring was saying.

whoops.. edited

profxj · 2017-09-21T19:20:18Z

Ok, give this a thumbs up (if you agree).

ntejos · 2017-09-21T19:23:24Z

I still do not understand the idea behind unique_components(), but I guess is fine.

profxj · 2017-09-21T19:39:12Z

Merging.

@ntejos -- You'll see this new method in use shortly. (z1IGM)

Note, I suspect igmguesses will break again. But I'll help fix it.

coveralls · 2017-09-21T20:37:30Z

Coverage increased (+0.1%) to 71.756% when pulling fd2005b on unique_components into 21556df on master.

profxj added 5 commits September 17, 2017 12:52

more component work

2298245

refactor LineLimits -> zLimits

fdc22d1

refactor complete?

aa28c88

test fix

1fa79c8

scrub GaussianAbsorption1D

d1efe3d

profxj requested a review from jnburchett September 18, 2017 22:55

jnburchett requested changes Sep 20, 2017

View reviewed changes

ntejos requested changes Sep 21, 2017

View reviewed changes

addressed NT, JB comments

baade8c

utils updated

03a1cba

jnburchett reviewed Sep 21, 2017

View reviewed changes

a few more edits

fd2005b

ntejos approved these changes Sep 21, 2017

View reviewed changes

jnburchett approved these changes Sep 21, 2017

View reviewed changes

profxj merged commit 681e086 into master Sep 21, 2017

profxj deleted the unique_components branch September 21, 2017 19:39

		@@ -23,7 +24,8 @@ class LineLimits(object):
		zlim : tuple of floats

Unique components #410

Unique components #410

Conversation

profxj commented Sep 18, 2017

coveralls commented Sep 18, 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

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 • edited Loading

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 21, 2017

ntejos commented Sep 21, 2017

coveralls commented Sep 21, 2017

coveralls commented Sep 21, 2017

jnburchett commented Sep 21, 2017 via email • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profxj commented Sep 21, 2017

ntejos commented Sep 21, 2017

profxj commented Sep 21, 2017

coveralls commented Sep 21, 2017

ntejos left a comment •

edited

Loading

jnburchett commented Sep 21, 2017 via email •

edited

Loading