Hi all,
For the record this approach isnt 100% backwards compatible, because
names(mergeddf) will e incompatibly different. Thatx why i claimed
bakcwards compatable-ish
That said its still worth considering imho because of the reasons stated
(and honestly one particular simple reading of the docs might suggest that
this was thr intended behavior all along). Im not a member of Rcore through
so i cant do said considering myself.
Best,
~G
On Feb 20, 2018 7:15 PM, wrote:
Hi Scott,
I tried the new patch and can confirm that it has the advertised
behavior on a couple of test cases. I think it makes sense to apply
it, because any existing code which refers to a second duplicate
data.frame column by name is already broken, while if the reference is
by numerical index then changing the column name shouldn't break it.
I don't know if you need to update the documentation as part of your
patch, or if whoever applies it would be happy to do that. Somebody
from R core want to weigh in on this?
I attach a file with the test example from your original email as well
as a second test case I added with two "by" columns.
Thanks,
Frederick
On Wed, Feb 21, 2018 at 10:06:21AM +1100, Scott Ritchie wrote:
> Hi Frederick,
>
> It looks like I didn't overwrite the patch.diff file after the last edits.
> Here's the correct patch (attached and copied below):
>
> Index: src/library/base/R/merge.R
> ===
> --- src/library/base/R/merge.R (revision 74280)
> +++ src/library/base/R/merge.R (working copy)
> @@ -157,6 +157,14 @@
> }
>
> if(has.common.nms) names(y) <- nm.y
> +## If by.x %in% names(y) then duplicate column names still arise,
> +## apply suffixes to just y - this keeps backwards compatibility
> +## when referring to by.x in the resulting data.frame
> +dupe.keyx <- intersect(nm.by, names(y))
> +if(length(dupe.keyx)) {
> + if(nzchar(suffixes[2L]))
> +names(y)[match(dupe.keyx, names(y), 0L)] <- paste(dupe.keyx,
> suffixes[2L], sep="")
> +}
> nm <- c(names(x), names(y))
> if(any(d <- duplicated(nm)))
> if(sum(d) > 1L)
>
> Best,
>
> Scott
>
> On 21 February 2018 at 08:23, wrote:
>
> > Hi Scott,
> >
> > I think that's a good idea and I tried your patch on my copy of the
> > repository. But it looks to me like the recent patch is identical to
> > the previous one, can you confirm this?
> >
> > Frederick
> >
> > On Mon, Feb 19, 2018 at 07:19:32AM +1100, Scott Ritchie wrote:
> > > Thanks Gabriel,
> > >
> > > I think your suggested approach is 100% backwards compatible
> > >
> > > Currently in the case of duplicate column names only the first can be
> > > indexed by its name. This will always be the column appearing in by.x,
> > > meaning the column in y with the same name cannot be accessed.
Appending
> > > ".y" (suffixes[2L]) to this column means it can now be accessed, while
> > > keeping the current behaviour of making the key columns always
accessible
> > > by using the names provided to by.x.
> > >
> > > I've attached a new patch that has this behaviour.
> > >
> > > Best,
> > >
> > > Scott
> > >
> > > On 19 February 2018 at 05:08, Gabriel Becker
> > wrote:
> > >
> > > > It seems like there is a way that is backwards compatible-ish in the
> > sense
> > > > mentioned and still has the (arguably, but a good argument I think)
> > better
> > > > behavior:
> > > >
> > > > if by.x is 'name', (AND by.y is not also 'name'), then x's 'name'
> > column
> > > > is called name and y's 'name' column (not used int he merge) is
> > changed to
> > > > name.y.
> > > >
> > > > Now of course this would still change output, but it would change
it to
> > > > something I think would be better, while retaining the 'merge
columns
> > > > retain their exact names' mechanic as documented.
> > > >
> > > > ~G
> > > >
> > > > On Sat, Feb 17, 2018 at 6:50 PM, Scott Ritchie <
s.ritchi...@gmail.com>
> > > > wrote:
> > > >
> > > >> Thanks Duncan and Frederick,
> > > >>
> > > >> I suspected as much - there doesn't appear to be any reason why
> > conflicts
> > > >> between by.x and names(y) shouldn't and cannot be checked, but I
can
> > see
> > > >> how this might be more trouble than its worth given it potentially
may
> > > >> break downstream packages (i.e. any cases where this occurs but
they
> > > >> expect
> > > >> the name of the key column(s) to remain the same).
> > > >>
> > > >> Best,
> > > >>
> > > >> Scott
> > > >>
> > > >> On 18 February 2018 at 11:48, Duncan Murdoch <
> > murdoch.dun...@gmail.com>
> > > >> wrote:
> > > >>
> > > >> > On 17/02/2018 6:36 PM, frede...@ofb.net wrote:
> > > >> >
> > > >> >> Hi Scott,
> > > >> >>
> > > >> >> Thanks for the patch. I'm not really involved in R development;
it
> > > >> >> will be up to someone in the R core team to apply it. I would
> > hazard
> > > >> >> to say that even if correct (I haven't checked), it will not be
> > >
