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 ValueError for unknown densities #2775

Merged
merged 3 commits into from Aug 25, 2021

Conversation

adriente
Copy link
Contributor

@adriente adriente commented Jun 23, 2021

Description of the change

Some entries of the elements database have unknown densities due to their very high radioactivity : Astate and Francium. When calling the function _density_of_mixture from hyperspy.misc.material with these elements it would raise a TypeError with a difficult to understand message.
With this PR, the function raises a ValueError with a more understandable error message.

Progress of the PR

  • Change implemented
  • ready for review.
  • make a test.
  • make a changelog entry.

Minimal example of the bug fix or the new feature

import hyperspy.misc.material as m
m._density_of_mixture([1.0],["At"])

New output

Traceback (most recent call last):
  File "/home/teurtrie/hyperspy/hyperspy/misc/material.py", line 224, in _density_of_mixture
    sum_densities[i] = weight / densities[i]
TypeError: unsupported operand type(s) for /: 'float' and 'numpy.str_'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/teurtrie/hyperspy/hyperspy/misc/material.py", line 236, in _density_of_mixture
    raise ValueError(
ValueError: The density of one of the elements is unknown (Probably At or Fr).

@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #2775 (fa3e3eb) into RELEASE_next_minor (2ddc359) will increase coverage by 0.30%.
The diff coverage is 93.75%.

❗ Current head fa3e3eb differs from pull request most recent head fcf0fe8. Consider uploading reports for the commit fcf0fe8 to get more accurate results
Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2775      +/-   ##
======================================================
+ Coverage               78.14%   78.45%   +0.30%     
======================================================
  Files                     203      203              
  Lines                   30681    31281     +600     
  Branches                 6704     6871     +167     
======================================================
+ Hits                    23976    24540     +564     
- Misses                   4963     4978      +15     
- Partials                 1742     1763      +21     
Impacted Files Coverage Δ
hyperspy/misc/material.py 83.78% <93.75%> (+0.33%) ⬆️
hyperspy/_signals/dielectric_function.py 86.11% <0.00%> (-4.80%) ⬇️
hyperspy/misc/array_tools.py 84.37% <0.00%> (-2.62%) ⬇️
hyperspy/datasets/artificial_data.py 98.64% <0.00%> (-1.36%) ⬇️
hyperspy/_signals/complex_signal.py 93.78% <0.00%> (-1.15%) ⬇️
hyperspy/utils/model_selection.py 81.81% <0.00%> (-0.80%) ⬇️
hyperspy/models/model1d.py 87.96% <0.00%> (-0.71%) ⬇️
hyperspy/misc/utils.py 85.90% <0.00%> (-0.38%) ⬇️
hyperspy/io_plugins/hspy.py 68.93% <0.00%> (-0.37%) ⬇️
hyperspy/signal.py 75.92% <0.00%> (-0.34%) ⬇️
... and 60 more

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 2ddc359...fcf0fe8. 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, it needs a test and to make the PR to the RELEASE_next_patch branch because it is a bug fix.
To avoid pulling change from the RELEASE_next_minor branch, the base branch will need to changed, see https://hyperspy.readthedocs.io/en/stable/dev_guide/git.html#changing-base-branch for explanation. If you prefer, I can sort it out before merging.

density = sum_densities / sum_weight
return np.where(sum_weight == 0.0, 0.0, density)
except TypeError :
raise ValueError(
Copy link
Member

Choose a reason for hiding this comment

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

A test is needed to cover this case, you can simply add your example from #2775 in

def test_density_of_mixture():
.

@ericpre ericpre linked an issue Jun 23, 2021 that may be closed by this pull request
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.

It needs a changelog entry in the upcoming_changes folder too - see upcoming_changes/README.rst!

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, looks good. A few commits are unnecessary and I will remove them before merging.

@ericpre ericpre merged commit 87aa9c9 into hyperspy:RELEASE_next_minor Aug 25, 2021
@ericpre ericpre added this to the v1.7 milestone Aug 25, 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.

Error : Minor issue with density_of_mixture
2 participants