Skip to content

Fix/gui cache dir#442

Open
fadybishara wants to merge 4 commits intomasterfrom
fix/gui-cache-dir
Open

Fix/gui cache dir#442
fadybishara wants to merge 4 commits intomasterfrom
fix/gui-cache-dir

Conversation

@fadybishara
Copy link
Contributor

@fadybishara fadybishara commented Jan 5, 2026

Changed the scope of the use_cache option in the SpectrometerCalibration method so that it doesn't attempt to create a directory or save the averaged data into it.

The reason behind this is that I was trying to open a run from XMPL where I have read but not write access (even to ${proposal}/scratch). This problem could come up in other cases as well, not just with XMPL, if a DA person has access to a proposal but is not part of it.

There are other alternatives/possible additions to the proposed propsoed including:

  1. Allow the user to pass-in a numpy array (like the out= option in extra.data) [@takluyver ]
  2. Silently fall back to a writeable directory if user cannot write to ${proposal}/scratch. For example, ${HOME}/.cache/extra or something like that.
  3. Allow the user to pass-in a path where they do have write access. This has the advantage of giving the user control over where the cached files are saved so that they can delete them or pass the directory back to read from cache.

These options were discussed on Zulip with @turkot, @philsmt, and @takluyver.

@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.88%. Comparing base (e6d1244) to head (fbc7a72).

Files with missing lines Patch % Lines
src/extra/gui/jupyter/spectrometer_calibration.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
- Coverage   72.89%   72.88%   -0.02%     
==========================================
  Files          35       35              
  Lines        6091     6092       +1     
==========================================
  Hits         4440     4440              
- Misses       1651     1652       +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.

@takluyver
Copy link
Member

other alternatives/possible additions ... Allow the user to pass-in a numpy array

To be clear, that's a feature that's already there (image_data=), rather than a proposed addition.

Comment on lines +366 to +367
cache_path.mkdir(mode=666, exist_ok=True, parents=True)
np.save(cache_path / fname, mean_data)
Copy link
Member

Choose a reason for hiding this comment

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

While we're changing this, I might be inclined to catch and ignore PermissionError here, so you don't need to pass use_cache=False for XMPL proposals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like this? Inside the if use_cache or do you want to get rid of the use_cache=False option altogether?

Suggested change
cache_path.mkdir(mode=666, exist_ok=True, parents=True)
np.save(cache_path / fname, mean_data)
try:
cache_path.mkdir(mode=666, exist_ok=True, parents=True)
np.save(cache_path / fname, mean_data)
except PermissionError:
pass

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that's what I was thinking. I'd leave the use_cache option around as well - you might want to skip using the cache even if it works, e.g. if you suspect it might be outdated.

@fadybishara
Copy link
Contributor Author

fadybishara commented Jan 5, 2026

To be clear, that's a feature that's already there (image_data=), rather than a proposed addition.

Hmm, I don't think quite as we discussed unless I missed something? The documentation suggests that image_data is an input, nothing gets saved to it but you can pass an array instead of loading data from a file. I thought we were talking about saving the average of the loaded data from a run, or did I misunderstand?

Usage

Provide either:

A proposal, run_number and data source: the data will be a average image of the run.
a 2D numpy array as image_data

image_data will be used first if provided. If data is loaded from a run, the generated average will be cached at ${Proposal}/scratch/.EXtra-gui-jupyter-SpectrometerCalibration-cache

Co-authored-by: Thomas Kluyver <thomas.kluyver@xfel.eu>
@fadybishara fadybishara requested a review from tmichela January 5, 2026 18:53
@takluyver
Copy link
Member

The documentation suggests that image_data is an input, nothing gets saved to it but you can pass an array instead of loading data from a file.

That's right: I meant that it gives you the option to load & average the data separately, so then you can save/load a prepared array yourself rather than relying on the caching behaviour.

Copy link
Member

@tmichela tmichela left a comment

Choose a reason for hiding this comment

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

+1 to @takluyver 's comment.

Could you also add a clear_cache method? Now that the cache can't be updated anymore, we need a way to remove it, other than navigating the filesystem.

Changed:

- [SpectrometerCalibration][extra.gui.jupyter.SpectrometerCalibration] no longer
saves loaded data to cache if `user_cache=False`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
saves loaded data to cache if `user_cache=False`.
saves loaded data to cache if `use_cache=False`.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants