The Source Code for NYC's Forensic DNA Statistical Analysis Tool

https://github.com/propublica/nyc-dna-software

by foob • 9 years ago

134 45 9 years ago

foob• 9 years ago

I haven't had a chance to dig into the analysis part of the code yet, but here's an amusing variable assignment that jumped out at me in `FST.Common/ComparisonData.cs`.

    compareMethodIDSerializationBackupDoNotMessWithThisVariableBecauseItsAnUglyHackAroundACrappyLimitiationOfTheFCLsCrappyXMLSerializer = comparisonID;

thirstysusrando• 9 years ago

Link for those interested:

https://github.com/propublica/nyc-dna-software/blob/master/F...

slackingoff20176• 9 years ago

I love this type of thing in C# because it's such a terse and well designed language. It's easy to make something stand out.

A cult favorite of devs at my previous employer was to (ab)use the power of LINQ to try to create the longest single line methods possible. I held the unofficial record for a while with a LINQ statement over 1200 chars. Innocent enough since resharper can unwrap them into sane code but lots of fun

sixothree• 9 years ago

I love/hate it when someone writes a linq statement and ends with "look, one line".

mcescalante• 9 years ago

The accompanying article/blog post linked in the Readme (https://www.propublica.org/article/federal-judge-unseals-new...) has good background & information about the history of this, as well as potential impact moving forward.

inetknght4• 9 years ago

I work in DNA analysis and stuff like this makes me happy specifically because I know just how easy it is to have flawed analysis techniques written in software.

CyberDildonics3• 9 years ago

How long does this stuff typically take to run on a dataset?

inetknght4• 9 years ago

That depends on the specific kind of analysis being done and one what size of dataset you want to run the analysis. Some analysis techniques filters data down to just a few MiB, others need as much as a few hundred GiB, for a single individual. Sometimes you do population analysis on that data which can end up multiplying individual sample size to the size of the population.

devillighter• 9 years ago

Code in "Comparison.cs" on line 1307

   // if we don't have frequencies, use the default (this should never happen, but here it is)
                    if (tblFreq == null || tblFreq.Rows.Count == 0)
                    {
                        a = (float)0.02;
                        b = (float)0.02;
                    }

is kind of fishy. Why not bomb out instead of failing silently for things that are never supposed to happen? Not saying this is inherently wrong - I'm certainly not capable of actually understanding the details of the analysis

sjg007• 9 years ago

Unless it’s a uniform prior... this is bad.

trendia• 9 years ago

The only thing that should follow a "This should never happen" is an assert or exception.

slackingoff20176• 9 years ago

This guy that wrote this is a c# noob. He should just use a null conditional for the if, Epsilon for tiny but non-zero floats, and should very likely be using doubles instead of float since they're basically the same speed for double the precision.

Bonus points for not using the shorthand float literal syntax.

I would expect to see this many goofs only from someone that's been using C# for a month or so. I only used it for a couple years

TwoBit• 9 years ago

Your criticism seems a bit overboard. You are making assumptions about things you don't know about, and are judging personal choice floating point representation.

tootie• 9 years ago

Do we know who wrote this or at least which city agency? Or was it done by consultants or acquired?

finnn• 9 years ago

It seems that the source code for any computer program who's output is used as evidence in a court of law should be available for inspection.

tekmaven2• 9 years ago

On first glance it uses very old frameworks (for example, ASP.NET WebForms) and there are 0 unit tests.

A concerning thing is that a dev database username and password was committed to multiple files, like this: https://github.com/propublica/nyc-dna-software/blob/master/F....

bas• 9 years ago

For this kind of project, given the stakes involved (e.g. a defendant's freedom), why would you want to use the brand-new framework-of-the-month that the cool kids are using?

michaelmrose• 9 years ago

Possibly they shouldn't be using bug ridden trash either seems like a compromise ought to be possible.

tekmaven2• 9 years ago

I'm all for using the right tool for the job. WebForms code is not possible / extremely difficult to unit test.

The actual codebase itself may be pretty old so WebForms isn't a surprise - the SLN file indicates it was saved with Visual Studio 2010 (https://github.com/propublica/nyc-dna-software/blob/338a1b86...).

slackingoff20176• 9 years ago

For something contracted this is the norm. We used to send out estimates for writing tests and clients never wanted to pay for them. I've found projects just as buggy with tests and now I only write them for code that's getting heavily reused.

Webforms is likely a client request as well, but also keep in mind that as shitty as webforms is it let's you throw functionality together faster than almost anything else. Again proly contractors

The funny comments others mentioned is another clue that this was not developed internally. I miss that about being a consulting company... The client is usually hiring you because they have no idea what they're doing so rest assured nobody there will ever look at the code.

gravypod• 9 years ago

Is there a large dataset of anonymized DNA samples that we could try this tool against? Does 23andme do something like that? I hope they don't but it would be very useful for just this one case!

TechnicalVault• 9 years ago

23andMe use a genotyping array so they probably don't type STRs only SNPs

AlexCoventry4• 9 years ago

Amazingly, it looks like they still use Sanger sequencing, and a lot of the uncertainty will probably come from genotyping errors.

There is a lot of anonymized genotype data in the 1000 Genomes Project (http://www.internationalgenome.org/data), but the devil is really in the genotyping details with this kind of thing. It looks like in the validation project (https://www.documentcloud.org/documents/4113877-1-19-17-Exhi...), they went from biological samples to sequence data to analysis results, which is the right approach.

With this kind of low-througput genotyping so much depends on the accuracy of the genotyping method, though, which in turn depends on the lab, the protocol, the technicians, and sometimes even the weather. The software is not where I would start, in worrying about forensic evidence based on this method, though it definitely could be a source of errors.

dekhn2• 9 years ago

I'm not sure what you mean by "amazingly, it looks like they still use Sanger sequencing". Sanger is considered gold standard-- all HTS genome methods are validated against it.

Alex3917• 9 years ago

For context, this is getting released just after the city council committee on technology had a public hearing about a bill that would require any software used by the government to prosecute folks for crimes to be open source:

https://www.fwdeveryone.com/t/FrtKpHJ2T0mCic3j_FSzPQ/nyc-sof...

This bill would set the standard for the entire country, so the testimony is actually pretty interesting and worth watching, especially the opening speech by councilman Vacca.

https://councilnyc.viebit.com/player.php?hash=xHL4m7vQXCM9

I also testify at 56:00, albeit hadn't prepared remarks in advance so wasn't as polished as some of the other folks.

slackingoff20176• 9 years ago

This is excellent news and I'm incredibly grateful the code used to convict people will be publicly scrutinized.

That said, me and other C# devs have already found numerous signs that at least parts of the codebase were done by someone clueless or inexperienced with C#. And there's no tests.

If someone spends the time to look into the codebase, I'm predicting a basically 100% chance that parts of this code will be shown to produce inaccurate or completely wrong results. Innocent people will probably go free because of this.

jeffshek• 9 years ago

The fact that there are no tests makes me feel incredibly sad - there are probably plenty of people who were unfairly sentenced because of this.

SeanDav• 9 years ago

You make a very valid and telling point early on when you point out that in NYC, a person in a job painting people's nails (Nail Technician/Cosmetologist) requires around 1000 hours of training, yet a software developer designing software that can put people in jail (or worse) requires no formal training or qualification at all!

Disclaimer - I mean no disrespect to Nail Technicians.

jasminz• 9 years ago

I just love the way they have connection string along with what seems real database URI and accompanying username and password.

tmsam• 9 years ago

Nested ternaries, ungodly amounts of repetition, ZERO tests, hardcoded sensitive info... this code wouldn't pass code review anywhere. I don't care when it was written, any day it is used to send people to jail it should be the best possible code humans can write. And it is so far from that.

The Source Code for NYC's Forensic DNA Statistical Analysis Tool

Related Stories