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-ENH update cutoff_at_kV behavior for bruker.py reader #2910

Merged
merged 7 commits into from Mar 29, 2022

Conversation

sem-geologist
Copy link
Contributor

@sem-geologist sem-geologist commented Mar 25, 2022

Description of the change

Changes how bruker.py reader reacts to cutoff_at_kV:

  • None as value does no more any cutoff
  • introduced possible value "zealous" which cuts at last nonzero value
  • previous behavior with None can be activated with value "auto", with updated fallback if hv=0

This partly fixes the #2898

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring
  • update user guide,
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add/modify tests,
  • ready for review.

* `None` does no cutoff
* introduced acceptance of "auto" which cuts at last nonzero value
* previous behavior with None can be activated with "lowest_constraint" value
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #2910 (3c54e79) into RELEASE_next_minor (6e2ddbb) will increase coverage by 0.18%.
The diff coverage is 81.25%.

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2910      +/-   ##
======================================================
+ Coverage               79.37%   79.56%   +0.18%     
======================================================
  Files                     208      209       +1     
  Lines                   31939    32270     +331     
  Branches                 7194     7242      +48     
======================================================
+ Hits                    25353    25677     +324     
- Misses                   4837     4839       +2     
- Partials                 1749     1754       +5     
Impacted Files Coverage Δ
hyperspy/io.py 86.45% <ø> (+0.69%) ⬆️
hyperspy/io_plugins/bruker.py 88.04% <81.25%> (-0.80%) ⬇️
hyperspy/io_plugins/__init__.py 71.42% <0.00%> (ø)
hyperspy/io_plugins/tvips.py 99.07% <0.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 6e2ddbb...3c54e79. Read the comment docs.

fix some size vs index +1/-1 issues
updated cutoff_at_kV under Bruker Composit Files
adoption of exmples to cutoff_at_kV change
@sem-geologist
Copy link
Contributor Author

sem-geologist commented Mar 25, 2022

looks that fixing the behaviour of cutoff_at_kV makes it behaves similarly to jeol reader where None does not do any hideous auto cutting, I think I should also now address and change docstring of common reader. I see it also has downsampling. I think there could be separate PR to look through all of readers and find same keywords to update. I think this PR is complete.

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.

Looks good, I have left some questions/suggestions.

hyperspy/io_plugins/bruker.py Outdated Show resolved Hide resolved
hyperspy/io.py Outdated Show resolved Hide resolved
@sem-geologist
Copy link
Contributor Author

@ericpre , what do you think about the changes made? ("auto" to "last_non-zero"; "lowest_constraint" to "em_hv")

doc/user_guide/io.rst Outdated Show resolved Hide resolved
@ericpre
Copy link
Member

ericpre commented Mar 29, 2022

Thanks, this looks good to me!
Would it be possible to clean the git history by squashing some of the commits? For example, in this PR, there are some iterative changes and for our future self, typically when browsing the git history, it is convenient not to have commits which do/undo changes.

@sem-geologist
Copy link
Contributor Author

sem-geologist commented Mar 29, 2022

squashed last 12. Is it enough? I was unfortunately mobile and I don't know how on github I could edit few files and commit a bunch. Thus such an amount of commits...

@ericpre ericpre merged commit cbb6be0 into hyperspy:RELEASE_next_minor Mar 29, 2022
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