Skip to content

Conversation

@ericpre
Copy link
Member

@ericpre ericpre commented Jul 19, 2025

@francisco-dlp, as discussed in #3525 (comment), this is the PR to deprecate tmp_parameters in favour of extending https://hyperspy.org/hyperspy-doc/current/reference/metadata.html#fileio.

The filename, folder and extension will now be stored in https://hyperspy.org/hyperspy-doc/current/reference/metadata.html#fileio and the metadata of the last load operation will be used to define the parameters previously saved in tmp_parameters.

Corresponding rosettasciio PR: hyperspy/rosettasciio#425

Progress of the PR

  • Fix a regression with overwritting zspy with directory stores - introduced in Improve file saving functionality and deprecate obsolete parameters #3525,
  • Deprecate the poorly documentation tmp_parameters in favour of FileIO metadata
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

@ericpre ericpre changed the title Move tmp parameters Deprecate tmp_parameters in favour of FileIO metadata Jul 19, 2025
@codecov
Copy link

codecov bot commented Jul 19, 2025

Codecov Report

❌ Patch coverage is 83.56164% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.90%. Comparing base (1386c49) to head (eeb2d45).
⚠️ Report is 57 commits behind head on RELEASE_next_minor.

Files with missing lines Patch % Lines
hyperspy/signal.py 84.21% 5 Missing and 4 partials ⚠️
hyperspy/io.py 81.25% 3 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #3528      +/-   ##
======================================================
+ Coverage               81.88%   81.90%   +0.02%     
======================================================
  Files                     149      149              
  Lines                   22916    22912       -4     
  Branches                 4673     4675       +2     
======================================================
+ Hits                    18764    18767       +3     
+ Misses                   2951     2945       -6     
+ Partials                 1201     1200       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ericpre ericpre force-pushed the move_tmp_parameters branch from 27f97f7 to c82a1c6 Compare July 20, 2025 10:52
@ericpre ericpre added this to the v2.4.0 milestone Jul 20, 2025
@ericpre ericpre requested a review from Copilot July 20, 2025 10:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR deprecates the poorly documented tmp_parameters in favor of the FileIO metadata system for storing file operation information. The change improves consistency and documentation while maintaining backward compatibility through deprecation warnings.

Key Changes

  • Deprecates tmp_parameters property with warning messages pointing to metadata.General.FileIO
  • Updates file I/O operations to store filename, folder, and extension information in FileIO metadata
  • Refactors save/load functionality to use the new metadata structure instead of tmp_parameters

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
hyperspy/signal.py Deprecates tmp_parameters property and updates save/load logic to use FileIO metadata
hyperspy/io.py Updates file I/O operations to populate FileIO metadata instead of tmp_parameters
hyperspy/tests/test_io.py Updates tests to use new FileIO metadata structure and removes deprecated functionality
hyperspy/tests/signals/test_tools.py Removes deprecated tmp_parameters usage in tests
hyperspy/tests/drawing/test_markers.py Updates test to use FileIO metadata for filename manipulation
doc/user_guide/io.rst Updates documentation to reference FileIO metadata instead of tmp_parameters
doc/reference/metadata.rst Adds documentation for new FileIO metadata fields
upcoming_changes/*.rst Updates changelog entries to reflect the deprecation

Copy link
Member

@francisco-dlp francisco-dlp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise this LGTM. However, there are subtle but important differences in the behaviour of the feature in respect to the previous implementation that raise issues that must be address.

tmp_parameters is not copied when e.g. slicing a signal, while metadata is. Also, tmp_parameters, unlike metadata, is not stored when saving a file. This was intentional (hence the name tmp_parameters: the file location in the file system was stored only "temporarily" because:

  • It is not related to the data itself
  • Files can move
  • The transformed file (e.g. a slice) is not typically saved in a way that replaces the original file. (Of course there are methods that transform the data in-place, which is an issue also for the tmp_parameters implementation.
  • The path contains user information, that the user may not like to store in the file

----------
string : str
File extension, without initial "." separator
Format name or file extension, with or without initial "." separator.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not sticking only to one of the two (with our without). In HyperSpy we have typically gone for without, but in Python it is a lot more common to include the dot in the extension. Therefore, why not supporting both, with a warning that from v3.0 only the extension with dot will be supported?

@ericpre
Copy link
Member Author

ericpre commented Jul 25, 2025

Thank you @francisco-dlp for the review. I did think about these differences and thought that these can be considered as undocumented implementation details, while the functionality stay the same.
The benefit of keeping this information in the metadata is to help with traceability, for example, knowing when a file was converted, etc. I don't see these "implementation details" changes as particularly problematic.

What would you suggest? Not to use FileIO and only privatise tmp_parameters with deprecation?
While I don't have strong opinion either way, in this PR, I have leaned toward using FileIO, because of the traceability benefits. At the same time, I agree that the path to the file isn't really a property of the data... I am happy to go for the privatisation of tmp_parameters only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants