Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LD prune bug with 1kg data #6223

Open
tpoterba opened this issue May 30, 2019 · 12 comments

Comments

Projects
None yet
2 participants
@tpoterba
Copy link
Collaborator

commented May 30, 2019

bug report here: https://discuss.hail.is/t/ld-prune-starts-and-stops-error/

@jbloom22 any ideas? You've worked on this stuff a bit

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2019

@catoverdrive randomly assigned you from scorecard middle-end

@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

So it looks like locally_pruned_table ends up with an extra locus that's not in mt? I'm not sure how that happens, though, since it's literally the result of the LocalLDPrune table function. I'm not super familiar with how that works, but I'll poke at it for a little bit.

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2019

Unfortunately Dan and Jon are the two most familiar, and they're both OOO.

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2019

WTF - I'm getting this error in a PR - https://ci.hail.is/jobs/30344/log

also see this message at the bottom:

2019-05-30 19:47:48 Hail: WARN: struct{idx: int32} has no field row_idx at <root>.<array>.end for value JInt(10)
@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2019

my pr had an obvious bug -- but I think this issue may be related to duplicate keys

@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

This is running into the split_multi issue. it would be running into this error in the RVD iterator, but we don't perform this check too many places anymore nowadays and it's not hitting this check anywhere after the split happens.

              if (localType.kRowOrd.gt(prevK.value, rv)) {
                kUR.set(prevK.value)
                val prevKeyString = kUR.toString()

                prevK.setSelect(localType.rowType, localType.kFieldIdx, rv)
                kUR.set(prevK.value)
                val currKeyString = kUR.toString()
                fatal(
                  s"""RVD error! Keys found out of order:
                     |  Current key:  $currKeyString
                     |  Previous key: $prevKeyString
                     |This error can occur after a split_multi if the dataset
                     |contains both multiallelic variants and duplicated loci.
                   """.stripMargin)
              }
@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

Going to think about what the right thing to do here is. Maybe split_multi should have a check, even if it makes things slower?

@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

The really weird thing is that I think even a write/read/collect doesn't trigger this error.

@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

to reproduce:

l = hl.Locus('1', 1)
rows = [
	hl.Struct(locus=l, alleles=["A", "T"]),
	hl.Struct(locus=l, alleles=["T", "T"]),
	hl.Struct(locus=l, alleles=["A", "CC", "TT"])]

t = hl.Table.parallelize(rows, hl.tstruct(locus=hl.tlocus(), alleles=hl.tarray(hl.tstr)), ['locus', 'alleles'])
split = hl.split_multi(t)
print(split.collect())

should error, but does not. Writing/reading/showing split should also error, but instead prints:

2019-06-04 18:55:11 Hail: INFO: wrote table with 4 rows in 1 partition to foo
+---------------+------------+---------+-----------+---------------+-----------------+
| locus         | alleles    | a_index | was_split | old_locus     | old_alleles     |
+---------------+------------+---------+-----------+---------------+-----------------+
| locus<GRCh37> | array<str> |   int32 |      bool | locus<GRCh37> | array<str>      |
+---------------+------------+---------+-----------+---------------+-----------------+
| 1:1           | ["A","T"]  |       1 |     false | 1:1           | ["A","T"]       |
| 1:1           | ["T","T"]  |       1 |     false | 1:1           | ["T","T"]       |
| 1:1           | ["A","CC"] |       1 |      true | 1:1           | ["A","CC","TT"] |
| 1:1           | ["A","TT"] |       2 |      true | 1:1           | ["A","CC","TT"] |
+---------------+------------+---------+-----------+---------------+-----------------+
@catoverdrive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

ah---we totally turned off key checking.

    if (!HailContext.get.checkRVDKeys)
      return new RVD(typ, partitioner, crdd)
@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2019

This is definitely no longer high-prio -- we'll throw the correct error message now.

@tpoterba tpoterba removed the prio:high label Jun 11, 2019

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2019

I remember Arcturus having some idea about a fuller solution, so I will let them close the issue when ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.