Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Neaspec multichannel reader #739

Merged
merged 7 commits into from
Oct 3, 2024

Conversation

brnovasco
Copy link
Contributor

This PR is related to the discussion in #717.

Following up on the development of a reader for the Neaspec data containing multiple channels with separated data for phase and amplitude in multiple runs, rows and columns.

We are creating a reader that performs a interpolation of the data to be able to have all interferograms in the same domain related to the raw position sensor reading instead of its array index.

This imply changes in the OWFFT widget so we avoid duplicate code for the complex fft calculation. We then standardize the way the widget performs the fft for the .gsf and .txt data from Neaspec simplifying the logic on the FFT widget and improving the NeaReaderGSF class.

@stuart-cls
Copy link
Member

Thanks!
This seems quite clear, @raul-freitas explained to me about the interpolation part but I won't comment on that part.

This imply changes in the OWFFT widget so we avoid duplicate code for the complex fft calculation. We then standardize the way the widget performs the fft for the .gsf and .txt data from Neaspec simplifying the logic on the FFT widget and improving the NeaReaderGSF class.

This is very nice, thank you.

From the FFT side, this PR seems good. Perhaps @markotoplak can put a second set of eyes on the reader side.

@markotoplak markotoplak changed the title Neaspec multichannel reader [ENH] Neaspec multichannel reader Sep 27, 2024
Copy link
Collaborator

@markotoplak markotoplak left a comment

Choose a reason for hiding this comment

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

I only have complaints about the test file size and this is the only thing I will enforce.

I do not deal with such files ofter therefore I am not the one to judge this from usability point of view. Also, someone else has to decide what to do with many Nea readers we know have - are there any to remove or merge together?

@markotoplak
Copy link
Collaborator

@borondics, @stuart-cls, @brnovasco, please tell me when this is ready - I am not a good judge of file formats. If I understood the above comments, row and column should be changed to map_x and map_y, and then this is it?

Please do not merge yourselves because the test file should be made smaller and the big one rebased out.

@borondics
Copy link
Member

My only wish here is to have "map_x" and "map_y" as meta names.

Maybe we could make a definition file that all orange-spectroscopy can import and define these there?

In some other readers the final step is a helper function that constructs the data table from the different parts and that has "map_x" and "map_y". So, we could unify things that way too.

Also, in this particular case it should be row -> map_x and column -> map_y because the measurement method is row-by-row, which introduces special need for the data analysis.

As for test files, it should be really easy to just delete all the data except for a few lines, then it is a really dramatic space saving.

@stuart-cls stuart-cls mentioned this pull request Sep 27, 2024
@stuart-cls
Copy link
Member

stuart-cls commented Sep 27, 2024

Here's my understanding:

  • Test file too big
  • standard map vars would be nice
    - [ ] .txt extension is overloaded Deferred for solution to Neaspec readers #747
  • decide on meters vs microns

…le channels

Reader needs to resample the data to the same domain to compensate M position
deviations and comply with the orange-spectroscopy data model.
…he polar form (separated amplitude and phase channels in alternating rows).
….gsf format

Do so by making the reader calculate its datapoint spacing and save it as
metadata that can be consumed in the fft widget. This allow the owfft widget to
treat the data from the NeaReaderGSF and the NeaReaderMultiChannel the same
way, as it should since they both use the same complex fft and provide data in
similar form.
… scaling the domain to [µm] and renaming column and row to standard map_x and map_y vars.
@brnovasco brnovasco force-pushed the neaspec-multichannel-reader branch from cb6b3a2 to 781b1d2 Compare October 2, 2024 19:14
@brnovasco
Copy link
Contributor Author

Hi, @markotoplak and @stuart-cls. I think its ready with the requested changes. Can you check?

@brnovasco brnovasco requested a review from markotoplak October 2, 2024 19:21
Copy link
Member

@stuart-cls stuart-cls left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for this nice PR.

Copy link
Collaborator

@markotoplak markotoplak left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Also thank you for rebasing the big file out.

@markotoplak markotoplak merged commit c80bcca into Quasars:master Oct 3, 2024
10 of 14 checks passed
@brnovasco brnovasco deleted the neaspec-multichannel-reader branch October 3, 2024 20:10
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.

5 participants