rosettasciio icon indicating copy to clipboard operation
rosettasciio copied to clipboard

app5 reader

Open maclariz opened this issue 6 months ago • 18 comments

Here is an app5 reader. This works on the app5 files I get from our NanoMegas topspin system. However, the formatting of the api might be totally different to how you prefer in hyperspy/rosettasciio. So that might need discussion. Anyway, this is a good place to start the discussion.

I could provide an app5 file as a tester if you want for further development

Requirements

  • Until RosettaSciIO has a contributor guide, you can read the HyperSpy developer guide to familiarise yourself with how to contribute.
  • Base your pull request on the correct branch (so far main branch)
  • Fill out the template; it helps the review process and it is useful to summarise the PR.
  • This template can be updated during the progression of the PR to summarise its status.

You can delete this section after you read it.

Description of the change

A few sentences and/or a bulleted list to describe and motivate the change: New reader for NanoMEGAS app5

Progress of the PR

  • [ X] Change implemented (can be split into several points),
  • [X ] update docstring (if appropriate),
  • [ ] update user guide (if appropriate),
  • [ ] add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • [ ] Check formatting of the changelog entry (and eventual user guide changes) in the docs/readthedocs.org:rosettasciio build of this PR (link in github checks)
  • [ ] add tests,
  • [ X] ready for review.

Minimal example of the bug fix or the new feature

from rsciio.msa import file_reader
file_reader("your_msa_file.msa")
# Your new feature...

Note that this example can be useful to update the user guide.

maclariz avatar Jun 19 '25 16:06 maclariz

Thank you @maclariz for starting this pull request. @argerlt for your awarness since you mention your interest in this contributing this in https://github.com/hyperspy/rosettasciio/issues/405.

There is some documentation on how to add new plugin: https://hyperspy.org/rosettasciio/contributing.html#defining-new-rosettasciio-plugins.

  • there is a structure to follow to make the API consistent (definition of file_reader, file_writer and their argument and what they return) but the implementation itself, there is no specific requirement, you can do as you wish as long you comply the API.
  • it will need tests and tests files. The test files should be as small as possible and most likely mean to create some test file specially for this task. There is some recommendations at https://hyperspy.org/rosettasciio/contributing.html#making-test-data-files. The test files are added to the repository and the registry needs to be updated accordingly: https://hyperspy.org/rosettasciio/contributing.html#adding-and-updating-test-data.

If something is unclear or blocking you, please let us know.

ericpre avatar Jun 22 '25 08:06 ericpre

Another important things: as a new contributor adding a plugin, please let us know if there are any steps that would benefit from better documentation, sign-posting, etc.

ericpre avatar Jun 22 '25 09:06 ericpre

@ericpre , thanks for the heads up. I did still want to write an .app5 loader but got side-tracked and honestly, @maclariz 's looks better generalized than the method I've been using lately. I was also using some JSON-nonsense to parse the Metadata Group. this approach is much cleaner. If this commit works, I'm happy.

@maclariz, if you do ever want help, I can work though the failing workflows and generate some fake data and unittests, but it looks like you have things handled without me.

Thank you for adding this! This will be very helpful for me and my colleagues!

argerlt avatar Jun 23 '25 16:06 argerlt

Codecov Report

Attention: Patch coverage is 8.33333% with 88 lines in your changes missing coverage. Please review.

Project coverage is 87.24%. Comparing base (2670e51) to head (a318d55). Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
rsciio/nanomegas_app5/_api.py 7.60% 85 Missing :warning:
rsciio/nanomegas_app5/__init__.py 25.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   87.90%   87.24%   -0.67%     
==========================================
  Files          89       91       +2     
  Lines       11464    11560      +96     
  Branches     2116     2128      +12     
==========================================
+ Hits        10077    10085       +8     
- Misses        878      966      +88     
  Partials      509      509              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Jun 23 '25 17:06 codecov[bot]

@argerlt Thanks. I have been using basically this to import data into py4dstem all Friday, so all the actual workings are well-checked. There were a couple of minor later commits on Friday to fix minor issues I found. There is still one, which is that the diffraction calibration is coming out wrong, so I have misinterpreted their numbers. I'll get to the bottom of this. I suspect it is an angle in degrees that needs converting to rad or mrad before converting to reciprocal units.

I'll spend more time later in the week trying to work out how to make it fit within the general shape of things used in this code project. I am still relatively inexperienced at things like that, although not bad at making code that actually works to do a specific job.

The other thing in the to-do is to find a way to pick up the series area box from the survey area image metadata. I am sure it must be there somewhere, I just haven't found it yet. Anyway, I think first priority is just get what I have working in hyperspy (possibly with fixed reciprocal space calibration) and then worry about tweaks like the survey rectangle later...

maclariz avatar Jun 23 '25 19:06 maclariz

@maclariz Do you need any help with this? I can check this out and help get all of the tests passing. (although maybe not before M&M)

CSSFrancis avatar Jul 14 '25 13:07 CSSFrancis

@maclariz, do you have any plan to continue this PR? It will need some test files and we can help with the rest. @argerlt, you mentioned that you can generate some test files; if you have the opportunity, could you please help with this?

The relevant sections of the documentation are:

  • https://hyperspy.org/rosettasciio/contributing.html#making-test-data-files
  • https://hyperspy.org/rosettasciio/contributing.html#adding-and-updating-test-data

ericpre avatar Nov 01 '25 12:11 ericpre

@argerlt, you mentioned that you can generate some test files; if you have the opportunity, could you please help with this?

Funny story, this PR just came up at work again, they asked if I could help get this PR finished by Nov 5th.

Yes, I can help. I will make some test files ASAP. Afterwards, I can also try to fix the remaining unit tests if they are still failing.

argerlt avatar Nov 01 '25 17:11 argerlt

@ericpre @argerlt Yes, very much want to get this finished. I know the base code works as I have used it repeatedly in the last months to import app5 files containing just one scan dataset.

However, the summer got very full as just before my holidays I got a very picky review on an important paper and it took ages sorting everything out. However, the glut of publications that rather took over my life through the summer is now almost out of the way (2 are accepted, and one about to go back with revisions).

I am happy to do what I can to get this out there.

maclariz avatar Nov 03 '25 10:11 maclariz

What is missing is some representative and small test files - see comment above for more explanation. Then we will review it/sort out the details.

@argerlt, if there is a pressing deadline and you would like to get this PR merged ASAP, maybe the best is that you start another PR based on this branch to make sure that @maclariz's commit are kept in the history. Since you are very familiar with contributing to open-source projects (and its workflow, etc.), I would expect that it will not take you long to get it ready to merge ✨👌! @maclariz, you can then watch and contribute to the review! 😃 In either way, having one of you contributing to the review will be good.

ericpre avatar Nov 04 '25 09:11 ericpre

My test files are not small

maclariz avatar Nov 04 '25 10:11 maclariz

Most of the time, test files needs to be acquired specifically for this purpose in order to make them small and test specific functionality, setup, etc. There are guideline in https://hyperspy.org/rosettasciio/contributing.html#making-test-data-files.

ericpre avatar Nov 04 '25 12:11 ericpre

@argerlt, if there is a pressing deadline and you would like to get this PR merged ASAP, maybe the best is that you start another PR based on this branch to make sure that @maclariz's commit are kept in the history.

Sounds reasonable, I will do so. I also have some 5Mb datasets I can upload.

argerlt avatar Nov 05 '25 23:11 argerlt

I'll hopefully have some time tomorrow to have another look and maybe get things updated. There is some general wrapping that need not be there. I think there is also a slight mistake in picking up the reciprocal space calibration.

At the moment, my method doesn't support lazy loading. Could be done. But probably easiest for first version to simply get something that works in simplest form possible, and then extend later. My two wishlist items for further future would then be:

  1. Lazy loading
  2. Working out how the hell to read out where the scan box is placed on the survey image. At the moment, I cannot work out from reading any tag I can get hold of how to do this. It must be in there somewhere, but that would be several hours of frustration of looking in every possible place in the tree structure for numbers that relate to the pixel positions where the corners of the box go.

maclariz avatar Nov 06 '25 09:11 maclariz

Oh, and: 3. Working out the meaning of the calibration for kx and ky. Haven't got the foggiest. They are floats. But I haven't learned what they mean yet.

maclariz avatar Nov 11 '25 19:11 maclariz

@ericpre @argerlt so, it's now failing on things that are nothing to do with anything I wrote and are literally unchanged from standard rosettasciio

maclariz avatar Nov 11 '25 20:11 maclariz

The test failure is genuine but this can be fixed it later. It needs tests, @argerlt you mentioned you have some small test files, are these already optimised (compressed with no or low signal, different shape, etc.) for adding to the test suite?

ericpre avatar Nov 15 '25 10:11 ericpre

@ericpre , I ended up pushing a new PR building off of this one, which includes some small .app5 files of a few different styles. it would be helpful to get a few others from different users, but that might be a "deal with it as it comes up" problem.

argerlt avatar Nov 18 '25 03:11 argerlt