Support the new ECOD distribution format - #1141
Open
aalhossary wants to merge 2 commits into
Open
Conversation
ECOD redesigned its distribution at v294.1 and neither the version comment nor the column layout is recognised any more. The version comment changed from "#ECOD version develop291" to "# Version: v295", so parsedVersion stayed null and getVersion() fell back to the string that was requested. The columns changed from 15 (or 16 in develop291, which inserts unp_acc) to 23 and then 25, so every data line in the current release is rejected and getAllDomains() quietly returns an empty list. Files that declare a column header - every release since develop101 - are now read by column name rather than by position, which covers all four layouts at once and will not need revisiting the next time a column moves. Older files keep the positional path unchanged. Along with the columns: - manual_rep now holds True/False rather than MANUAL_REP/AUTO_NONREP - assembly_id is empty on every row, meaning what NOT_DOMAIN_ASSEMBLY meant - the ligand list moved to ligand_comp_ids - the last column is empty on four rows in five, so trailing empty fields are kept rather than discarded by split() - 53% of rows now describe domains found in AlphaFold models, which have no PDB entry and so cannot be an EcodDomain; they are counted and reported rather than logged as errors Parsing nothing at all is now reported as an error instead of returning an empty list silently, and the per-line NumberFormatException warning is capped like the others rather than printing a stack trace per row. getVersion() reads only the file header. Parsing the current 657 MB release in full to answer that question costs over a gigabyte of heap, which does not fit comfortably in the 1500 MB the test JVM is given.
The distribution is 657 MB, so the only test that touched the current release was testVersion, which asserts on one string. Every other test pins develop204. That is why a wholesale format change surfaced as a single one-line assertion failure, eight months after it happened. EcodParserTest reads fixtures taken verbatim from the real files - 13, 15, 16, 23 and 25 columns, both version comment forms, a row ending in an empty column, an AlphaFold-derived row, and a malformed row - through the Reader constructor. Fifteen tests, no network, well under a second. testVersion additionally parses the first few thousand lines of the local file. That is enough to notice a column change without building the three million domains the whole file now holds.
Member
Author
|
CI result on this branch, as expected: five jobs fail, all on
That is the last remaining failure in the module, it is #1138, and #1133 fixes it — so with #1133 and this merged, The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1139.
ECOD redesigned its distribution at v294.1. Neither the version comment nor the column layout is recognised any more, so
EcodInstallationTest.testVersionfails and — less visibly — anyone callingEcodFactory.getEcodDatabase()today gets an empty domain list with no exception thrown.The version comment
#ECOD version develop291# Version: v295parsedVersionstayed null andgetVersion()fell back to the string that was requested, which forlatestis the literal"latest"— the value the test asserts against. One pattern now matches both forms, so the historical releases keep working; a regex that only accepted the new form would silently break every pinneddevelop*version.The columns
#uid …f_idrenamedt_id,unp_accinserted at 9#uid …uid …— no longer commented outligand_comp_ids,ligand_pdbnumuid …Rather than add a fourth positional special case, files that declare a column header — every release since develop101 — are now read by column name. That covers all four layouts at once and will not need revisiting the next time a column moves. Files older than develop101 have no header and keep the existing positional path, unchanged.
Everything else that moved with the columns:
manual_repholdsTrue/Falserather thanMANUAL_REP/AUTO_NONREP;assembly_idis declared but empty on all 2,945,500 rows, meaning whatNOT_DOMAIN_ASSEMBLYused to mean;ligand_comp_ids;ligand_pdbnum, the last column, is empty on 2,399,569 rows, sosplit("\t")was dropping it and making those rows look a column short. Trailing empty fields are now kept;f_nameis empty rather thanF_UNCLASSIFIEDfor unclassified domains — deliberately left as it is, see below;P44140_F1_nD2and an emptypdbcolumn.EcodDomainis keyed byPdbIdand cannot represent them, so they are counted and reported at INFO rather than each logged as an error.Two failure modes that hid this are also closed: parsing nothing at all is now logged as an error instead of quietly returning an empty list, and the per-line
NumberFormatExceptionwarning is capped like the three warnings beside it, rather than printing a stack trace per row.getVersion()no longer parses the whole fileIt reads the header. Parsing the current release in full to answer "which version?" costs 1,186 MB of heap and 9.8 s — and the test JVM is given
-Xmx1500Mby the root pom. The download is unchanged; only the parse is skipped. This is a small behaviour change, in thatgetVersion()no longer has the side effect of loading every domain, which the 7.3.0 target allows.Measured
ecod.develop124.domains.txtecod.develop204.domains.txtecod.develop291.domains.txtecod.latest.domains.txt(v295)The first two are the counts
EcodInstallationTestalready asserts, so the old path is provably untouched.Tests
EcodParserTestis new and entirely offline.EcodParseralready accepts aReader, so fixtures taken verbatim from the real files pin all five layouts, both version comment forms, a row ending in an empty column, an AlphaFold-derived row and a malformed row. 15 tests, no network, well under a second:That is the part I would most like kept. The distribution is 657 MB, so the only test that ever touched the current release was
testVersion, asserting on one string; everything else pinsdevelop204. That is exactly why a wholesale format change surfaced as a single one-line assertion failure, months after it happened.testVersionnow also parses the first few thousand lines of the local file — enough to notice a column change without building three million domains.9 tests, 1 skipped (
testAllVersions, already@Ignored for being slow), green in 124 s.Note on CI
Branched off
master, so itsPR Buildwill still show the CATH failure until #1133 merges. That is #1138, not something new here.Three judgement calls I would rather not make alone
@sbliven — you wrote this parser and know the format history far better than I do. If you have the time, I would value your eyes on it, particularly on:
unp_accis read and discarded. develop291 carries a UniProt accession thatEcodDomainhas nowhere to put. Adding a field means touchingequals/hashCode/toString/Comparable, so I left it out — but it is real data we are dropping.f_nameis left empty rather than translated toF_UNCLASSIFIED. They used to be equivalent; nowf_idclassifies to a fourth level (1.1.1.3) while the name is blank, so I do not think they are the same thing any more. If you disagree it is a one-line change.assembly_idis mapped onto the oldNOT_DOMAIN_ASSEMBLYsemantics (assemblyId = uid). It is empty on every row of v294+, so assembly information is simply gone from the distribution; this keepsgetAssemblyId()returning what callers expect rather than null.Also deliberately out of scope, and worth a follow-up issue rather than growing this PR:
valid_structure,ligand_binding,ligand_pdbnum,range_countand the five*_manualflags are new columns with no home inEcodDomain, and the AlphaFold half of the distribution needs a model that is not keyed byPdbId.Unrelated but useful
ECOD now publishes
ecod.latest.md5in coreutils format alongside the distribution. That is a realhashURLforFileDownloadUtils.createValidationFiles(url, file, hashURL, Hash.MD5)— the path #1133 repairs and which, before it, could never write a hash file at all. Not wired up here; noting it so it is not lost.