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

Perkin Elmer file reader #774

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

borondics
Copy link
Member

Based on existing code from specio and from colleagues at the Diamond synchrotron comes a new reader for Perkin Elmer files.

@borondics borondics requested a review from stuart-cls November 13, 2024 14:37
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.

Great!

You need to add the test files to MANIFEST.in before we can find out if the tests pass :)

Generally, I don't love the structure of the reader:

  1. It's two readers jammed into one for no reason I can see.
  2. I don't really like the API boundary between the specio code and the reader code. Looking at specio, I can see why it was done this way. I would prefer to keep the code-copies in the utils folder and the Orange-specific stuff only in the reader section.

I understand if you don't have much appetite for pedantic refactoring :) I don't think it will affect maintainability so skip it if you'd rather not.

orangecontrib/spectroscopy/io/__init__.py Outdated Show resolved Hide resolved
orangecontrib/spectroscopy/io/perkinelmer.py Outdated Show resolved Hide resolved
orangecontrib/spectroscopy/io/perkinelmer.py Outdated Show resolved Hide resolved
orangecontrib/spectroscopy/io/perkinelmer.py Outdated Show resolved Hide resolved
@borondics
Copy link
Member Author

Thanks for the comments guys, I tried to improve things! Have a look...

@markotoplak
Copy link
Collaborator

Before merging, this should be rebased into a single commit. Also, there is a file with CRLF (main lint's complain). This should be reformatted.

I can do this steps, when ready.

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.

This is good from my side, don't forget @markotoplak comment about line endings and squashing the commits.

@borondics
Copy link
Member Author

Then, it is good for me too... @markotoplak, would you merge it?

@markotoplak
Copy link
Collaborator

Tests are bad. Test should not skip complex parts of the code, like image coordinates, which are not tested at all. This should be improved.

@markotoplak
Copy link
Collaborator

I am merging this as is, but tests should really be made better.

@markotoplak markotoplak merged commit aab0f25 into Quasars:master Nov 20, 2024
9 of 14 checks passed
markotoplak added a commit that referenced this pull request Nov 20, 2024
@borondics
Copy link
Member Author

Thanks for merging! I added map coordinate tests in #776

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