Skip to content

Add general loader module#140

Merged
Max-Gamill merged 14 commits intomainfrom
maxgamill-sheffield/general-file-loader
May 6, 2025
Merged

Add general loader module#140
Max-Gamill merged 14 commits intomainfrom
maxgamill-sheffield/general-file-loader

Conversation

@Max-Gamill
Copy link
Copy Markdown
Collaborator

@Max-Gamill Max-Gamill commented May 2, 2025

This PR adds a general file loader into TopoStats AFMReader which handles any filepath and uses the file extention to ship it to the correct reader.

This does include a few funky additions changes:

  • The general loader takes a filepath and channel string, and returns a tuple of (image, px2nm).
  • As this is made for the Napari plugin we are developing, it needs to "pass along" the error messages to Napari, thus the channel errors are poorly handled by being returned as a tuple of (ValueError(<message>), None).
  • For .topostats files, it uses the channel input to identify any image keys ("image" and "image_original" from the file). These are also returned for the error message.
  • Functions, linting and tests are all there :)

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.44%. Comparing base (ac9753c) to head (02e0506).
Report is 126 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   74.62%   79.44%   +4.81%     
==========================================
  Files           8       11       +3     
  Lines         607      832     +225     
==========================================
+ Hits          453      661     +208     
- Misses        154      171      +17     

☔ 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.

@Max-Gamill Max-Gamill requested review from SylviaWhittle and ns-rse May 2, 2025 20:20
Copy link
Copy Markdown
Collaborator

@SylviaWhittle SylviaWhittle left a comment

Choose a reason for hiding this comment

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

this is great, thank you. a general loader also makes it easier for quickly loading images in notebooks when experimenting, so I'll get use out of this personally.

The code looks good, given it a bit of a rushed review as you'll want this wrapped up before next week I assume.

It might be nice to document this in the usage docs, however, that can be pushed to an issue instead of this PR, IMO, especially as this is just for Napari for now and may change.

my comments here are pedantic and not very useful. completely optional

Comment thread AFMReader/general_loader.py Outdated
Comment thread AFMReader/general_loader.py
Comment thread tests/test_general_loader.py Outdated
Comment thread tests/test_general_loader.py Outdated
Max-Gamill and others added 3 commits May 2, 2025 22:15
Co-authored-by: Sylvia Whittle <86117496+SylviaWhittle@users.noreply.github.com>
Co-authored-by: Sylvia Whittle <86117496+SylviaWhittle@users.noreply.github.com>
Co-authored-by: Sylvia Whittle <86117496+SylviaWhittle@users.noreply.github.com>
@Max-Gamill
Copy link
Copy Markdown
Collaborator Author

Thank you for the lightning quick review @SylviaWhittle and catching the lack of return typehints (although it seems I was too quick in my haste to accept them!

(Moving this so its easier to see)

except ValueError as e:
            logger.error(f"{e}")
            return (e, None)  # cheeky return of an image, px2nm-like tuple object to propagate error message to Napari

👍 we will just have to be careful how we handle this if used in other applications than Napari (which requires no errors raised)

Yep totally agree! I understand that we'd want software to fail early when it does and this really doesn't help that. The only other thing I can think of atm is to move this into a "napari-specific general loader" and have a "general general loader" of a similar structure but with less janky code, that perhaps can also return the metadata as this might be useful for other applications.

@SylviaWhittle
Copy link
Copy Markdown
Collaborator

Yeah maybe a napari specific loader later on, but for now this is great

@SylviaWhittle
Copy link
Copy Markdown
Collaborator

Ah, needs from __future__ import annotations at the top of general_loader.py to enable pipe to be allowed for types in Python 3.9

Comment thread AFMReader/general_loader.py
Comment thread AFMReader/gwy.py
Comment thread AFMReader/general_loader.py Outdated
Comment thread AFMReader/general_loader.py Outdated
Comment thread AFMReader/general_loader.py Outdated
Comment thread AFMReader/general_loader.py Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The general_ part is perhaps redundant, loader.py would suffice and be descriptive enough.

@ns-rse
Copy link
Copy Markdown
Collaborator

ns-rse commented May 6, 2025

Ah, needs from __future__ import annotations at the top of general_loader.py to enable pipe to be allowed for types in Python 3.9

As with #1146 in TopoStats I'd be inclined to set the minimum version of supported Python to 3.10 for AFMReader. Noted in #142.

@Max-Gamill Max-Gamill merged commit 8160e9a into main May 6, 2025
13 checks passed
@Max-Gamill Max-Gamill deleted the maxgamill-sheffield/general-file-loader branch May 6, 2025 17:02
@ns-rse ns-rse mentioned this pull request May 20, 2025
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.

4 participants