add pickle deprecation (fixes #42)#46
Draft
sahiljhawar wants to merge 14 commits intomainfrom
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a .pickle deprecation path to DataOrgStrategy, which is one of the legacy saving strategies in the codebase. It updates runtime behavior and API docs so callers are nudged toward .mat while keeping existing pickle support in place for now.
Changes:
- Add runtime deprecation warnings when
DataOrgStrategyis configured with.pickle. - Document pickle deprecation in the class, constructor, and
append_data()docstrings. - Reject
append_data()calls for non-.pickletargets with an explicitNotImplementedError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+85
to
+87
| .. deprecated:: 1.0.3rc0 | ||
| Passing ``".pickle"`` is deprecated and will be removed in a future release. | ||
| Use ``".mat"`` instead. |
Comment on lines
+89
to
+95
| if file_format == ".pickle": | ||
| warnings.warn( | ||
| "The '.pickle' file format for DataOrgStrategy is deprecated and will be removed " | ||
| "in a future release. Use '.mat' instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
Comment on lines
+242
to
+252
| if file_path.suffix != ".pickle": | ||
| msg = f"Appending to '{file_path.suffix}' files is not supported by DataOrgStrategy. Only '.pickle' files support appending, but that format is deprecated. `append_data` method for '{file_path.suffix}' is not implemented and will be followed in future releases." # noqa: E501 | ||
| logger.exception(msg) | ||
| raise NotImplementedError(msg) | ||
|
|
||
| warnings.warn( | ||
| "Appending to '.pickle' files is deprecated alongside the '.pickle' format and will " | ||
| "be removed in a future release. Switch to '.mat' to avoid this warning.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… make sure mat and pickle files have the same data (fixes #45)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+88
| .. deprecated:: 1.0.3rc0 | ||
| Passing ``".pickle"`` is deprecated and will be removed in a future release. | ||
| Use ``".mat"`` instead. |
Comment on lines
+90
to
+96
| if file_format == ".pickle": | ||
| warnings.warn( | ||
| "The '.pickle' file format for DataOrgStrategy is deprecated and will be removed " | ||
| "in a future release. Use '.mat' instead.", | ||
| FutureWarning, | ||
| stacklevel=2, | ||
| ) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…e_single_file method
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+195
to
+201
| if format_name == ".pickle": | ||
| msg = ( | ||
| "Pickle format has been removed from `SingleFileStrategy` and will be soon" | ||
| "removed from `SavingStrategy` as well (already deprecated)." | ||
| ) | ||
| logger.error(msg) | ||
| raise RuntimeError(msg) |
|
|
||
| def _append_mat_data(self, file_path: Path, data_dict_to_save: dict[str, Any]) -> dict[str, Any]: | ||
| """Load an existing MATLAB file and merge the new data into it.""" | ||
| data_dict_old = loadmat(str(file_path)) |
Comment on lines
+204
to
+215
| def _recursively_load_datasets(group: h5py.Group | h5py.File, prefix: str = "") -> None: | ||
| """Recursively load datasets from groups and subgroups.""" | ||
| for key, item in group.items(): | ||
| full_path = f"{prefix}{key}" if prefix else key | ||
| if isinstance(item, h5py.Dataset): | ||
| loaded_data[full_path] = np.array(item) | ||
| elif isinstance(item, h5py.Group): | ||
| _recursively_load_datasets(item, f"{full_path}/") | ||
|
|
||
| with h5py.File(file_path, "r") as file: | ||
| _recursively_load_datasets(file) | ||
|
|
Comment on lines
+262
to
+268
| for key in all_keys: | ||
| if key not in existing_data: | ||
| merged[key] = new_data[key] | ||
| continue | ||
|
|
||
| if key not in new_data: | ||
| merged[key] = existing_data[key] |
Comment on lines
+115
to
+124
| @abstractmethod | ||
| def save_single_file(self, file_path: Path, dict_to_save: dict[str, Any], *, append: bool = False) -> None: | ||
| """Saves the provided dictionary to a single file in one of the supported formats (.mat, .pickle, .h5, .nc). | ||
|
|
||
| Parameters: | ||
| file_path (Path): The path where the file should be saved. | ||
| dict_to_save (dict[str, Any]): The dictionary containing variable data and metadata to be saved. | ||
| append (bool, optional): If True, data will be appended to existing files rather than overwriting them. | ||
| Defaults to False. | ||
| """ |
Comment on lines
+86
to
+88
| .. deprecated:: 1.0.3rc0 | ||
| Passing ``".pickle"`` is deprecated and will be removed in a future release. | ||
| Use ``".mat"`` instead. |
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.
No description provided.