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

Check signal specification conflicts #2445

Open
wants to merge 3 commits into
base: RELEASE_next_minor
Choose a base branch
from

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Jul 18, 2020

For hyperspy/hyperspy-extensions-list#3, I think when we add an package to the extension list, it would be useful to check if this package introduce signal specifications conflicts. I will add tests and update the dev guide if we agree on the approach.

Progress of the PR

  • If several signal class match are found, raise a warning and set to a generic signal since we don't have a reason to set it to one or another,
  • add function to check for signal specification conflicts,
  • update dev guide,
  • add tests,
  • ready for review.

@ericpre ericpre force-pushed the check_signal_specification_conflicts branch from f456bc8 to 12fc1a2 Compare July 18, 2020 19:26
@tjof2
Copy link
Contributor

tjof2 commented Jul 18, 2020

Any risk of this conflicting with #2434? e.g. multiple warning messages or one branch of code never being called?

@ericpre
Copy link
Member Author

ericpre commented Jul 18, 2020

Yes, these changes should be conflicting with #2434. I guess, we should complete #2434 and then solve the merge conflict in this PR.

@tjof2
Copy link
Contributor

tjof2 commented Jul 18, 2020

That's waiting on hyperspy/hyperspy-extensions-list#2 so hopefully there's no circular dependency!!

…_signal_specification_conflicts

# Conflicts:
#	hyperspy/io.py

if not isinstance(signal_dimension, int):
raise ValueError(f'Data type "{dtype.name}" not understood!')
if not isinstance(signal_dimension, int) or signal_dimension < 0:
raise ValueError("signal_dimension must be an interger")
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo "integer". Also worth saying it must be >= 0?

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #2445 into RELEASE_next_minor will decrease coverage by 0.01%.
The diff coverage is 48.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2445      +/-   ##
======================================================
- Coverage               74.15%   74.13%   -0.02%     
======================================================
  Files                     197      197              
  Lines                   27889    27905      +16     
  Branches                 6042     6046       +4     
======================================================
+ Hits                    20681    20688       +7     
- Misses                   5498     5506       +8     
- Partials                 1710     1711       +1     
Impacted Files Coverage Δ
hyperspy/io.py 75.70% <45.83%> (-2.22%) ⬇️
hyperspy/utils/__init__.py 100.00% <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 ad52cb7...b2c14c9. Read the comment docs.

@tjof2 tjof2 added this to the v1.7 milestone Aug 22, 2020
@ericpre ericpre removed this from the v1.7 milestone Oct 27, 2021
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