Fix get_data() dropping files when a DataTableID has multiple URLs - #61
Merged
Conversation
Member
Author
|
Problem with failure of tests comes from the new Pangaea update and will require new fixes. Merging things for now. |
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.
This PR fixes #58
Problem
NOAADataset.get_data()assumed a 1:1 mapping between DataTableID and file URL — it only ever fetched paleo_data.file_url, a shortcut to the first file NOAA attached to that table, silently ignoring the rest. In practice, one DataTableID can carry several files of different formats (a NOAA-templated .txt, the original contributed .txt, a .csv, a directory-listing page, or proprietary tree-ring formats), and these aren't always the same table in another format — e.g. a tree-ring DataTableID can bundle a separate "Raw Measurements" table and a "Chronology" table under one ID. The old code returned at most one of them, and would outright crash if the first-listed file happened to be a non-.txt link (e.g. a folder).Separately, DataTableIDs were assumed unique; if the same ID ever appeared under two different Study/Site pairs, the internal index silently kept only the last one.
Fix
pyleotups/core/NOAADataset.pyget_data(dataTableIDs=...)now attempts every file under a DataTableID, ranked easiest-first (NOAA structured .txt → Original Contributed .txt → .csv → web page/folder link via new_rank_file_priority()), and keeps a DataFrame for every file that parses instead of stopping at the first success. A failing file is logged and skipped rather than aborting the rest; it only raises if none of a table's files could be parsed.Duplicate DataTableIDs(same ID across different Study/Site pairs) are now surfaced via_check_duplicate_datatable_ids(), called from bothget_tables()andget_data(). A newdata_table_index_all multi-map lets get_data()open files from every colliding Study/Site rather than only the one left in the old single-entry index._process_file()gains support for .csv (via pandas.read_csv) and for web pages / directory-listing links (no extension or .html): it fetches the page, extracts links with the stdlib html.parser (no new dependency), ranks them by the same priority, and attempts each — addressing the optional "detect folder, extract URLs" ask.Proprietary tree-ring formats (.crn, .rwl, .fhx, .lpd) remain explicitly unsupported and are still rejected, just now as one skipped attempt among several rather than a hard stop.
pyleotups/tests/test_NOAADataset.pyNew tests covering: priority ranking, falling back to the next file when the easiest one fails, raising when all files fail, keeping every DataFrame when multiple files succeed, duplicate-ID warnings in
get_tables()/get_data(), opening files from every colliding Study/Site, .csv parsing, and folder-page link extraction/parsing.Updated one existing assertion (test_get_data_t05_unsupported_file_type_raises) to match the more general "unsupported file type" message now that .csv and web pages are supported types.
Verification
Full suite: 121 passed (117 pre-existing + new tests), no regressions.
Checked against the live NOAA API, not just mocks:
NOAAStudyId=6400: 3 DataTableIDs each with a folder-link + an alternate .txt. Old code crashed on the very first ID (folder link has no .txt extension); fixed code returns all 3 successfully. This study was also added to the tutorials as an example.
xmlId=6400 (tree-ring study, DataTableID 6773, 5 files): now correctly returns 2 DataFrames (Chronology + Raw Measurements, the two NOAA-template .txt files) while clearly warning and skipping the 3 genuinely unparseable files (a prose correlation-stats report, .crn, .rwl).
Use of AI
The fix was generated by Claude (including new test suites). Human reviewed the verification against NOAA API manually.