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

added support for DPC #2567

Merged
merged 9 commits into from
Nov 3, 2020

Conversation

din14970
Copy link
Contributor

@din14970 din14970 commented Oct 29, 2020

Description of the change

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add tests,
  • ready for review.

@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #2567 into RELEASE_next_patch will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_patch    #2567      +/-   ##
======================================================
- Coverage               75.98%   75.98%   -0.01%     
======================================================
  Files                     202      202              
  Lines                   29600    29608       +8     
  Branches                 6451     6453       +2     
======================================================
+ Hits                    22493    22499       +6     
- Misses                   5309     5310       +1     
- Partials                 1798     1799       +1     
Impacted Files Coverage Δ
hyperspy/io_plugins/emd.py 65.21% <75.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37081fd...20d3fef. Read the comment docs.

@thomasaarholt
Copy link
Contributor

You're being very efficient :D I don't need it for now, but could you check if EDS datasets that also collect DPC can be read as well?

@din14970
Copy link
Contributor Author

I don't have such a dataset to try, but I would expect that it should also work. The DPC signals are all just stored as images in the Image group, they should be mostly independent from the SpectrumStream group. Then again, the Velox files can throw interesting curve-balls. This would be another example though of where lazy reading of the DPC image stack might be desirable.

@ericpre
Copy link
Member

ericpre commented Oct 30, 2020

@din14970, they should be some metadata to parse when loading this type of data? This would be good to update the title to something more useful than DF4?

@thomasaarholt, as mentioned by @din14970, reading these image is independent of reading the EDS stream. Is there anything in particular you have in mind?

@thomasaarholt
Copy link
Contributor

If they're independent, then all is well - that was my only concern.

@din14970
Copy link
Contributor Author

@ericpre yes this is a big problem, because unfortunately all DPC related signals come from the detector that Velox identifies as DF4 (at least on our microscope) so they all get the same title in hyperspy. In the metadata that accompanies the dataset I could not find anything that provides more context to the dataset. Instead, one can parse the info in I think the display or presentation group, that stores the display title velox uses and links to the dataset path. However, in this group there are only entries for those datasets that are open as a subwindow in Velox, which is not all the datasets in the hdf5 file. I would have to dig deeper to see if it is storing some additional info elsewhere.

@ericpre
Copy link
Member

ericpre commented Oct 31, 2020

din14970#1

@din14970
Copy link
Contributor Author

din14970 commented Nov 2, 2020

@ericpre The titles for DPC signals should now be fixed, it works at least on all the datasets I have. I added a minimal test emd file and a test which should cover most of the _get_detector_name function. Test file is still somewhat big (700 kb) because there are 8 datasets inside. All are only 2x2 but all the metadata add up.

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

I have left a few comments. I have tried with the "17_ 20191205 1.3 Mx Imaging Ceta 1347.emd" file, you shared in #2483 and I get a "Unrecognized_image_signal" title. I don't know what is this dataset supposed to be (it seems to the same as the one in the same file?)

Other than that, this looks good to me.

hyperspy/tests/io/test_emd.py Outdated Show resolved Hide resolved
hyperspy/tests/io/test_emd.py Show resolved Hide resolved
hyperspy/io_plugins/emd.py Outdated Show resolved Hide resolved
@din14970
Copy link
Contributor Author

din14970 commented Nov 3, 2020

@ericpre added some more conditions in the _get_detector_name function to catch FFT's and cross-correlation aligned datasets, but I suppose this is still not an exhaustive list and Thermo-Fisher can easily come up with additional operations that deserve different titles. All these will be caught with Unrecognized_image_signal.

I still have to find the best way to remove the initial emd file from the history, unfortunately I added it in the same commit as the first commit in this PR.

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

Great, thanks. Looks good to me!

@ericpre ericpre added this to the v1.6.1 milestone Nov 3, 2020
@din14970
Copy link
Contributor Author

din14970 commented Nov 3, 2020

note to self: add binary files as separate commits. This was quite the hassle.

@ericpre ericpre merged commit 6dc781f into hyperspy:RELEASE_next_patch Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants