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

fix complex rebin and add complex test #2789

Merged

Conversation

wstolp
Copy link
Contributor

@wstolp wstolp commented Jul 5, 2021

My second try to fix rebinning of complex signals, as suggested by @ericpre:

  • I added a test for complex data types.
  • I specified the data type during creation of the array that will receive the rebinned data.
  • To pass the test I had to rearrange the endianness conversion (don't know what this is tbh).

See discussion in #2788.

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #2789 (7b9bf09) into RELEASE_next_patch (3a07064) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           RELEASE_next_patch    #2789   +/-   ##
===================================================
  Coverage               77.36%   77.36%           
===================================================
  Files                     202      202           
  Lines                   30105    30105           
  Branches                 6579     6579           
===================================================
  Hits                    23290    23290           
  Misses                   5067     5067           
  Partials                 1748     1748           
Impacted Files Coverage Δ
hyperspy/misc/array_tools.py 83.59% <100.00%> (ø)

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 3a07064...7b9bf09. Read the comment docs.

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.

This looks good, can you please add an entry for the changelog as mentioned in the PR template - see https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/upcoming_changes/README.rst for more details?

@wstolp
Copy link
Contributor Author

wstolp commented Jul 6, 2021

Sorry I did not read that template attentively and now it's gone. Maybe it would be good to put that info in the developer documentation as well.
Anyway I added an RST file. Would that be all? :)

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.

Thanks!

@ericpre ericpre merged commit bea337d into hyperspy:RELEASE_next_patch Jul 6, 2021
@ericpre
Copy link
Member

ericpre commented Jul 6, 2021

Sorry I did not read that template attentively and now it's gone. Maybe it would be good to put that info in the developer documentation as well.

No worries! Yes, it would be good to see how this can be improved, but at the same time information should not be duplicated, because very often, one duplicate get outdated... Also as a new contributor, if you have any other comments on what is not clear, this is more than welcome!

Anyway I added an RST file. Would that be all? :)

Yes, this is merged now! We use zenodo to reference hyperspy and the contributor list is pulled automatically from the github contributor profile. If you want to be credited with your full name, you need to update your github profile. Adding affiliation may be good too, because we use it sometimes for conference abstract, etc.

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

2 participants