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

Fixing deprecation warning #114

Merged
merged 13 commits into from
Apr 29, 2022
Merged

Conversation

jordiferrero
Copy link
Contributor

@jordiferrero jordiferrero commented Mar 18, 2022

Description of the change

Try to fix depreciation warning #54 .
Depreciate the remove_background_from_file method. It will be replaced by a new better function using the HyperSpy syntaxis.

Progress of the PR

  • ready for review.

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2022

Codecov Report

Merging #114 (53ebdc7) into main (699be7d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #114   +/-   ##
=======================================
  Coverage   95.70%   95.70%           
=======================================
  Files          11       11           
  Lines         582      582           
=======================================
  Hits          557      557           
  Misses         25       25           
Impacted Files Coverage Δ
lumispy/signals/luminescence_spectrum.py 90.05% <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 699be7d...53ebdc7. Read the comment docs.

@jordiferrero
Copy link
Contributor Author

jordiferrero commented Mar 18, 2022

I believe the DepreciationWarning caused by luminescence_spectrum.py has been fixed.
There is still the following warning:

lumispy/tests/signals/test_luminescence_spectrum.py::TestLumiSpectrum::test_errors_raise
  /opt/hostedtoolcache/Python/3.9.10/x64/lib/python3.9/site-packages/numpy/core/fromnumeric.py:1970: VisibleDeprecationWarning: Creating an ndarray from ragged nested sequences (which is a list-or-tuple of lists-or-tuples-or ndarrays with different lengths or shapes) is deprecated. If you meant to do this, you must specify 'dtype=object' when creating the ndarray.
    result = asarray(a).shape

which seems to come from numpy, not us. Is that correct?

@jlaehne jlaehne requested a review from ericpre March 18, 2022 22:42
@jlaehne jlaehne added this to the v0.2.0 milestone Mar 18, 2022
@jlaehne jlaehne linked an issue Mar 18, 2022 that may be closed by this pull request
Copy link
Contributor

@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.

Looking at CI, the warning is still there. This PR slightly improves the syntax and make this function more readable.

It looks to me, that this method is not very clear; it would benefit from rewriting and possibly changing its behaviour:

  • the name remove_background_from_file implies that we would provide a file, but the docstring only mention arrays or Signal1D for the background argument
  • the flexibility in supporting different shape for the background argument is confusing.

As alternative to the current behaviour, I would suggest to support only Signal1D and add a separate method to read the background from a file, which returns a Signal1D. This would have the advantage of being transparent/explicit for the users.

I believe the DepreciationWarning caused by luminescence_spectrum.py has been fixed. There is still the following warning:

lumispy/tests/signals/test_luminescence_spectrum.py::TestLumiSpectrum::test_errors_raise
  /opt/hostedtoolcache/Python/3.9.10/x64/lib/python3.9/site-packages/numpy/core/fromnumeric.py:1970: VisibleDeprecationWarning: Creating an ndarray from ragged nested sequences (which is a list-or-tuple of lists-or-tuples-or ndarrays with different lengths or shapes) is deprecated. If you meant to do this, you must specify 'dtype=object' when creating the ndarray.
    result = asarray(a).shape

which seems to come from numpy, not us. Is that correct?

It most likely means that we are not using it as it is now expected to be used and there is some changes required in lumispy to remove this warning.

The description in #114 (comment) doesn't match with what this PR does! ;)

@jlaehne
Copy link
Contributor

jlaehne commented Mar 20, 2022

It looks to me, that this method is not very clear; it would benefit from rewriting and possibly changing its behaviour:

Indeed, creating a variable background that contains the x and y vectors and afterwards splitting it again is not very intuitive (and avoiding that would along the way solve the problem with the deprecation warning.

I agree with @ericpre that the "HyperSpy" way should be to provide a method where a Signal1D object is subtracted from another Signal1D object. That would make it much more valuable for others to reuse. If the axes don't match (in size, offset and scale for UniformDataAxis), the background signal should be rebinned/interpolated to the main signal axis. This would also ease an extension to work in non-uniform data axes in the future.

I would propose in fact writing a new method remove_background_signal to do the job, and adding a deprecation warning to this method in order not "surprise" anyone with the change.

@ericpre
Copy link
Contributor

ericpre commented Mar 20, 2022

I would propose in fact writing a new method remove_background_signal to do the job, and adding a deprecation warning to this method in order not "surprise" anyone with the change.

Other alternative would be:

  • rename remove_background_from_file remove_background to support the background argument and use remove_background of Signal1D when background, as it is currently doing. This pattern is used for various method, such plot, decomposition, etc.
  • implement it directly in the remove_background of Signal1D - thinking once more about, maybe the best way would be to add support for fitting a ScalableFixedPattern, as it would fit better in the flow of Signal1D.remove_background and this would allow to compare different background using the GUI.

@jlaehne
Copy link
Contributor

jlaehne commented Apr 15, 2022

As this was originally intended as a quick fix for some warnings, I would propose the following way forward

  • already add a deprecation warning to the existing function
  • create an issue that the improved functionality needs to be implemented (looks like the idea with the ScalableFixedPattern sounds most promising)
  • then merge this PR

@jlaehne jlaehne mentioned this pull request Apr 21, 2022
@ericpre
Copy link
Contributor

ericpre commented Apr 27, 2022

@jordiferrero, do you think, this is feasible to finish this PR for the 0.2 release, i. e. in the coming days?

@jordiferrero
Copy link
Contributor Author

I'll work on it tomorrow @ericpre
Sorry I've been out of coding for a while recently :-(

lumispy/signals/luminescence_spectrum.py Outdated Show resolved Hide resolved
@jordiferrero
Copy link
Contributor Author

@jlaehne Not it is ready for review. Can you maybe help me tell how to fix the "linting" error? That's something new to me :-)

@jlaehne jlaehne changed the title Fixing depreciation warning Fixing deprecation warning Apr 29, 2022
@jlaehne
Copy link
Contributor

jlaehne commented Apr 29, 2022

@jlaehne Not it is ready for review. Can you maybe help me tell how to fix the "linting" error? That's something new to me :-)

  1. It is an issue with the black formatting. In this case probably too long lines and easy to fix. I usually run black xy.py on the failing files to fix it automatically.

@jlaehne
Copy link
Contributor

jlaehne commented Apr 29, 2022

You can also try to set up an automatic black formatting on your system. See also:
https://hyperspy.org/hyperspy-doc/current/dev_guide/coding_style.html
https://kikuchipy.org/en/stable/contributing.html#code-style

@jlaehne jlaehne merged commit 183225b into LumiSpy:main Apr 29, 2022
@jordiferrero jordiferrero deleted the depreciation_warning branch April 30, 2022 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test deprecation warning
4 participants