[Bioc-devel] Noticed some XDouble* functionality missing in IRanges

[Hervé Pagès]

Hi Steve,

On 11-06-09 08:48 PM, Steve Lianoglou wrote:
> Hi,
>
> 2011/6/9 Hervé Pagès<hpages at fhcrc.org>:
>> On 11-06-09 10:59 AM, Steve Lianoglou wrote:
>>>
>>> On Thu, Jun 9, 2011 at 1:16 PM, Michael Lawrence
>>> <lawrence.michael at gene.com>    wrote:
>>>>
>>>>
>>>> On Thu, Jun 9, 2011 at 10:00 AM, Steve Lianoglou
>>>> <mailinglist.honeypot at gmail.com>    wrote:
>>>>>
>>>>> Greetings,
>>>>>
>>>>> I tried to slice a "normal" numeric/double vector and was a bit
>>>>> surprised to get an error.
>>>>>
>>>>> I see some support for doing this baked in, eg. the
>>>>> src/XDoubleViews_utils.c :: XDouble_slice function, but it's not
>>>>> exposed to the useR yet.
>>>>>
>>>>> Would anybody mind if I start building up support for XDouble's to be
>>>>> more inline with the support available in XIntegers? I'm just planning
>>>>> to add XDoubleView-class.R and XDoubleViews-utils.R files that
>>>>> essentially mimic their XIntegerView sister files.
>>>>>
>>>>> Out of curiosity, was there a reason this wasn't done other than no
>>>>> one had to scratch this itch yet?
>>>>
>>>> That's pretty much it. Just waiting for someone with the energy and a
>>>> use-case.
>>>
>>> Well, I've got at least one of those, so ... will do.
>>
>> Sounds good Steve. Yes please go ahead.
>
> I whipped this up and committed into IRanges revision 56093.

Thanks for this!

>
> I essentially took all of the R and C code for XInteverView stuff and
> ported it to work over XDouble's. All seems to work as expected and I
> put basic unit tests into  inst/unitTests/test_XDoubleViews.R
>
> A few comments for feedback:
>
> (1) One point that's different in the XDouble is inside the
> XDoubleViews-class::XDoubleViews.equal function. The result
> (TRUE/FALSE) value saved in each ans[i] is wrapped in a call to
> `all()` since X{Double|Integer}Views.view1_equal_view2 returns a
> logical vector which is as long as the elements that are passed into
> that function.
>
> If it's not wrapped in `all()` like it currently is in
> XIntegerViews.equal, then you get a warning that you are trying to
> assign a logical vector to 1 position (in `ans`) and only the first
> value is taken -- which I think is wrong. So, I think this change
> should be applied to the same position in the `XIntegerViews.equal`
> function, but I didn't do it since I thought I might be missing
> something.

Good catch. I fixed XIntegerViews.equal().

>
> (2) The XDoubleViews_viewSums function in XDoubleViews_utils.c is
> different from its sister XInteger function in that it's currently not
> checking for an overflow in the accumulated sum(s). My C is a bit
> rusty and I wasn't sure how to correctly do so here ... could someone
> please tweak and fix? It's on line 184-187 XDoubleViews_utils.cf .

Note that the check for overflow in XIntegerViews_viewSums() is broken.

My understanding is that when summing double values there is no need
to check for an overflow because the built-in arithmetic for doubles
will produce an inf (which is a special double value), and operations
involving an inf are well defined.

I checked your new code and discovered that we had some long-standing
issues with the pre-existing XIntegerViews code that you mimicked.
I fixed them but overall I think there is still lot of room for
improving the XIntegerViews/XDoubleViews code (I've added comments
where I saw possible improvements). In particular more code should
be shared, especially at the C level.

Thanks again for your contribution,
H.

>
> Thanks,
> -steve
>


-- 
Hervé Pagès

Program in Computational Biology
Division of Public Health Sciences
Fred Hutchinson Cancer Research Center
1100 Fairview Ave. N, M1-B514
P.O. Box 19024
Seattle, WA 98109-1024

E-mail: hpages at fhcrc.org
Phone:  (206) 667-5791
Fax:    (206) 667-1319