feat: add .json.gz support and fix pandas 2.0 compatibility - #991
Closed
lyraluoyu wants to merge 1 commit into
Closed
feat: add .json.gz support and fix pandas 2.0 compatibility#991lyraluoyu wants to merge 1 commit into
lyraluoyu wants to merge 1 commit into
Conversation
|
Thanks for opening this pull request! We have detected this is the first time you have contributed to NiMARE. Please check out our contributing guidelines. Of course, if you want to opt out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
Contributor
Reviewer's GuideAdds gzip-compressed JSON support to Dataset loading, hardens generic column access against Pandas 2.x behavior changes, and adjusts image dataframe utilities for compatibility with Series/DataFrame return types while adding a regression test for compressed dataset loading. Sequence diagram for Dataset initialization with .json.gz supportsequenceDiagram
actor User
participant Dataset
participant gzip
participant json
User->>Dataset: __init__(source_json_gz)
Dataset->>Dataset: isinstance_source_str
alt source_endswith_gz
Dataset->>gzip: open(source_json_gz, rt)
gzip-->>Dataset: f_obj
else source_not_gz
Dataset->>Dataset: open(source, r)
Dataset-->>Dataset: f_obj
end
Dataset->>json: load(f_obj)
json-->>Dataset: data_dict
Dataset->>Dataset: initialize_internal_state_with_data
Updated class diagram for Dataset and utils compatibility changesclassDiagram
class Dataset {
+Dataset(source, target, mask)
+get_labels(ids)
_generic_column_getter(attr, ids, column, ignore_columns)
_id_cols
}
class Utils {
+_validate_images_df(image_df)
}
Flow diagram for updated _generic_column_getter logicflowchart TD
A[Start_generic_column_getter] --> B[Compute_all_cols_excluding_id_cols]
B --> C{Is_column_provided}
C -- Yes --> D[Find_matches_equal_or_prefix]
D --> E{Matches_found}
E -- No --> F[Raise_ValueError_with_available_types]
E -- Yes --> G[Set_target_col_first_match]
G --> H[subset = df_target_col]
H --> I{ids_is_not_None}
I -- Yes --> J[subset = subset_loc_df_id_in_ids]
I -- No --> K[Keep_subset]
J --> L{subset_is_DataFrame}
K --> L
L -- Yes --> M[result = subset_iloc_first_column_tolist]
L -- No --> N[result = subset_tolist]
M --> R[Check_return_first_and_result_list]
N --> R
C -- No --> O[available_types = all_cols_excluding_ignore_columns]
O --> P[Init_empty_result_dict]
P --> Q[For_each_v_in_available_types]
Q --> S[subset = df_v]
S --> T{ids_is_not_None}
T -- Yes --> U[subset = subset_loc_df_id_in_ids]
T -- No --> V[Keep_subset]
U --> W{subset_is_DataFrame}
V --> W
W -- Yes --> X[result_v_first_column_tolist]
W -- No --> Y[result_v_subset_tolist]
X --> Z[Next_v_or_continue]
Y --> Z
Z --> AA[result = keys_with_any_non_None_values]
AA --> R
R --> AB{return_first_and_result_non_empty_list}
AB -->|Yes| AC[Return_first_element]
AB -->|No| AD[Return_result]
AC --> AE[End]
AD --> AE[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_validate_images_df, replacing theapply(lambda x: x.split(shared_path)[1] ...)logic withimage_df[abs_col].iloc[:, 0]both assumes a DataFrame (will error for a Series) and drops the relative-path transformation entirely, which likely breaks the function’s core purpose; consider restoring the path-splitting while still handling the Pandas 2.x DataFrame-return case defensively. - The new column resolution in
_generic_column_getterthat treatscolumnas a prefix (c == column or c.startswith(column)) can introduce ambiguous matches when multiple columns share a prefix; consider either requiring exact matches or raising an error when more than one column matches instead of silently takingmatches[0].
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_validate_images_df`, replacing the `apply(lambda x: x.split(shared_path)[1] ...)` logic with `image_df[abs_col].iloc[:, 0]` both assumes a DataFrame (will error for a Series) and drops the relative-path transformation entirely, which likely breaks the function’s core purpose; consider restoring the path-splitting while still handling the Pandas 2.x DataFrame-return case defensively.
- The new column resolution in `_generic_column_getter` that treats `column` as a prefix (`c == column or c.startswith(column)`) can introduce ambiguous matches when multiple columns share a prefix; consider either requiring exact matches or raising an error when more than one column matches instead of silently taking `matches[0]`.
## Individual Comments
### Comment 1
<location path="nimare/utils.py" line_range="590" />
<code_context>
- image_df_out[abs_col + "__relative"] = image_df[abs_col].apply(
- lambda x: x.split(shared_path)[1] if isinstance(x, str) else x
- )
+ image_df_out[abs_col + "__relative"] = image_df[abs_col].iloc[:, 0]
image_df = image_df_out
</code_context>
<issue_to_address>
**issue (bug_risk):** New relative-path logic both breaks on Series and drops the shared_path trimming behavior.
`image_df[abs_col]` returns a Series, so `.iloc[:, 0]` will raise `IndexError: Too many indexers`. Even if changed to `.iloc[0]`, this only copies the absolute path instead of computing a path relative to `shared_path`, so `__relative` is no longer a relative path.
To fix this, preserve the relative-path computation while handling both Series and DataFrame cases, e.g. by:
- Keeping the previous `split(shared_path)[1]` behavior (or a more robust `os.path.relpath`), and
- Explicitly branching for Series vs DataFrame rather than just copying the original value.
As written, this introduces both a runtime error (for the usual Series case) and a behavior regression.
</issue_to_address>
### Comment 2
<location path="nimare/dataset.py" line_range="523-528" />
<code_context>
+ all_cols = [c for c in df.columns if c not in self._id_cols]
if column is not None:
+ matches = [c for c in all_cols if c == column or c.startswith(column)]
+ if not matches:
+ raise ValueError(
+ f"{column} not found in {attr}.\nAvailable types: {', '.join(all_cols)}"
+ )
+ target_col = matches[0]
+
+ subset = df[target_col]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Prefix-based column matching can be ambiguous and silently pick the wrong column.
Because multiple columns can share a prefix (e.g., `"z"`, `"z_desc-consistency"`, `"z_desc-contrast"`), this logic may silently select the first match in `all_cols`, making behavior order-dependent and surprising. Instead of using `matches[0]`, consider either raising an error when `len(matches) > 1` and asking callers to disambiguate, or requiring an exact match in that case.
Suggested implementation:
```python
all_cols = [c for c in df.columns if c not in self._id_cols]
if column is not None:
matches = [c for c in all_cols if c == column or c.startswith(column)]
if not matches:
raise ValueError(
f"{column} not found in {attr}.\nAvailable types: {', '.join(all_cols)}"
)
# Prefer exact match if available; otherwise allow a single unambiguous prefix match.
if column in matches:
target_col = column
elif len(matches) == 1:
target_col = matches[0]
else:
# Multiple prefix matches and no exact match: require the caller to disambiguate.
raise ValueError(
"Ambiguous column specification "
f"{column!r} for {attr}. "
f"Matches: {', '.join(matches)}. "
"Please provide a more specific or exact column name."
)
subset = df[target_col]
```
```python
if ids is not None:
subset = subset.loc[df["id"].isin(ids)]
if isinstance(subset, pd.DataFrame):
result = subset.iloc[:, 0].tolist()
else:
result = subset.tolist()
else:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| image_df_out[abs_col + "__relative"] = image_df[abs_col].apply( | ||
| lambda x: x.split(shared_path)[1] if isinstance(x, str) else x | ||
| ) | ||
| image_df_out[abs_col + "__relative"] = image_df[abs_col].iloc[:, 0] |
Contributor
There was a problem hiding this comment.
issue (bug_risk): New relative-path logic both breaks on Series and drops the shared_path trimming behavior.
image_df[abs_col] returns a Series, so .iloc[:, 0] will raise IndexError: Too many indexers. Even if changed to .iloc[0], this only copies the absolute path instead of computing a path relative to shared_path, so __relative is no longer a relative path.
To fix this, preserve the relative-path computation while handling both Series and DataFrame cases, e.g. by:
- Keeping the previous
split(shared_path)[1]behavior (or a more robustos.path.relpath), and - Explicitly branching for Series vs DataFrame rather than just copying the original value.
As written, this introduces both a runtime error (for the usual Series case) and a behavior regression.
Comment on lines
+523
to
+528
| matches = [c for c in all_cols if c == column or c.startswith(column)] | ||
| if not matches: | ||
| raise ValueError( | ||
| f"{column} not found in {attr}.\nAvailable types: {', '.join(all_cols)}" | ||
| ) | ||
| target_col = matches[0] |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Prefix-based column matching can be ambiguous and silently pick the wrong column.
Because multiple columns can share a prefix (e.g., "z", "z_desc-consistency", "z_desc-contrast"), this logic may silently select the first match in all_cols, making behavior order-dependent and surprising. Instead of using matches[0], consider either raising an error when len(matches) > 1 and asking callers to disambiguate, or requiring an exact match in that case.
Suggested implementation:
all_cols = [c for c in df.columns if c not in self._id_cols]
if column is not None:
matches = [c for c in all_cols if c == column or c.startswith(column)]
if not matches:
raise ValueError(
f"{column} not found in {attr}.\nAvailable types: {', '.join(all_cols)}"
)
# Prefer exact match if available; otherwise allow a single unambiguous prefix match.
if column in matches:
target_col = column
elif len(matches) == 1:
target_col = matches[0]
else:
# Multiple prefix matches and no exact match: require the caller to disambiguate.
raise ValueError(
"Ambiguous column specification "
f"{column!r} for {attr}. "
f"Matches: {', '.join(matches)}. "
"Please provide a more specific or exact column name."
)
subset = df[target_col] if ids is not None:
subset = subset.loc[df["id"].isin(ids)]
if isinstance(subset, pd.DataFrame):
result = subset.iloc[:, 0].tolist()
else:
result = subset.tolist()
else:
Contributor
Author
|
Closing this PR for now — I realized the issue originates from how the compressed JSON test data is generated rather than the dataset logic itself. I'll open a new PR focusing on fixing the test data generation. |
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
1 participant
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.
Title: feat: support .json.gz format and fix Pandas 2.x compatibility in Dataset getter
Description:
This PR addresses two key areas of the Dataset module:
Gzip Support: Updated Dataset.load() and Dataset.save() to support .json.gz files. It now automatically detects the .gz extension and uses gzip for I/O operations, significantly reducing storage footprints for large meta-analytic datasets.
Pandas 2.x Compatibility: Fixed a regression where _generic_column_getter would crash with AttributeError: 'DataFrame' object has no attribute 'tolist'.
Root Cause: In newer Pandas versions (2.x), certain internal indexing or column access patterns within NiMARE's utilities can return a DataFrame instead of a Series (e.g., when duplicate column names are present or during specific slice operations).
Fix: Implemented a defensive check using isinstance(subset, pd.DataFrame). If a DataFrame is returned, we now explicitly select the first column via .iloc[:, 0] before calling .tolist().
Testing:
Passed all 22 tests in nimare/tests/test_dataset.py.
Verified that .json.gz loading/saving works as expected.
Confirmed backward compatibility with standard .json files and older Pandas environments.
Summary by Sourcery
Add gzip-compressed JSON support to Dataset loading and improve robustness and compatibility of DataFrame column handling across the codebase.
New Features:
Bug Fixes:
Tests: