search

LumiSpy / lumispy

Luminescence data analysis with HyperSpy.
https://lumispy.org
GNU General Public License v3.0
26 stars 17 forks source link

Crop edges update #142

Closed jordiferrero closed 1 year ago

jordiferrero commented 1 year ago

Description of the change

Following the request to extend the functionality of crop_edges from #129. I have implemented the following changes:

Progress of the PR

codecov-commenter commented 1 year ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (fec2c82) compared to base (cc5507b). Patch coverage: 100.00% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #142 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 12 12 Lines 545 470 -75 ========================================= - Hits 545 470 -75 ``` | [Impacted Files](https://codecov.io/gh/LumiSpy/lumispy/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LumiSpy) | Coverage Δ | | |---|---|---| | [lumispy/utils/axes.py](https://codecov.io/gh/LumiSpy/lumispy/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LumiSpy#diff-bHVtaXNweS91dGlscy9heGVzLnB5) | `100.00% <ø> (ø)` | | | [lumispy/signals/common\_luminescence.py](https://codecov.io/gh/LumiSpy/lumispy/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LumiSpy#diff-bHVtaXNweS9zaWduYWxzL2NvbW1vbl9sdW1pbmVzY2VuY2UucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LumiSpy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LumiSpy)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jordiferrero commented 1 year ago

Does lint correct formatting when PR is merged? I have tried to correct it manually but I cannot seem to get it working without an error...

Also tests are failing but not due to these changes. I will fix the test failures in a different PR

lgtm-com[bot] commented 1 year ago

This pull request introduces 1 alert when merging 3d4a98a177b5e8dd9d2e0e948b614c4fb7bf66e6 into 34fb15c62d84d0fd568684cd1ce02fff05fa09bf - view on LGTM.com

new alerts:

jlaehne commented 1 year ago

Does lint correct formatting when PR is merged? I have tried to correct it manually but I cannot seem to get it working without an error...

No, it is only doing the check. But you can run black locally to automatically reformat a file accordingly, see https://hyperspy.org/hyperspy-doc/current/dev_guide/coding_style.html

jlaehne commented 1 year ago

Currently, the function works only for maps, but you do not check whether the object actually has 2 navigation dimensions. To be more general, it should also work for linescans (number or 2-tuple) and throw an error if it is a single spectrum.

ericpre commented 1 year ago

Would it be worth fixing the test suite first?

How different is it from doing the following?

# remove one pixel on both side
s.isig[1:-1]

# remove one units on both side
s.isig[1.0:-1.0]

# remove one eV on both side
s.isig["1.0 eV":"-1.0eV"]

The last two seem to be broken but would be expected to work. The same approach would work for inav. The name crop_edges is ambiguous as it is not clear it it refers to navigation or signal space - my first intention was that it was in signal space.

jlaehne commented 1 year ago

Would it be worth fixing the test suite first?

Yes, would make the review easier.

The last two seem to be broken but would be expected to work. The same approach would work for inav. The name crop_edges is ambiguous as it is not clear it it refers to navigation or signal space - my first intention was that it was in signal space.

The last two might be broken for non-uniform axes? We should check that.

Indeed, it is a somewhat complicated wrapper for inav, which I personally have so far always used for this functionality (and we need to be clear that it is about the navigation axes). Added value of the latest implementation that I see would be that it accepts percentage values additionally and that the cropping is documented in the metadata.

ericpre commented 1 year ago

Added value of the latest implementation that I see would be that it accepts percentage values additionally and that the cropping is documented in the metadata.

I don't know if this is the kind of thing that should be documented in the metadata because I think that this is better to record this information in the script or the notebook itself. The issue with adding this kind of processing information to the metadata (other than the one which we know will be used for further processing, in which case, we can't get away with it) is that it is very brittle - difficult to enforce consistency and to maintain. More generally, this touch on the topic of data management, which is I believe is not the concern of library like lumispy, but rather the one of electronic laboratory notebook (ELN) and there are a lot of solution for this.

jordiferrero commented 1 year ago

Added value of the latest implementation that I see would be that it accepts percentage values additionally and that the cropping is documented in the metadata.

I don't know if this is the kind of thing that should be documented in the metadata because I think that this is better to record this information in the script or the notebook itself. The issue with adding this kind of processing information to the metadata (other than the one which we know will be used for further processing, in which case, we can't get away with it) is that it is very brittle - difficult to enforce consistency and to maintain. More generally, this touch on the topic of data management, which is I believe is not the concern of library like lumispy, but rather the one of electronic laboratory notebook (ELN) and there are a lot of solution for this.

The reason I wanted to document the cropping in the metadata is because CL data is always paired with SE data (for the SEM). The SE images are normally a different file and, if I want to plot a CL map next to the SE image, then I need to know what cropping was done exactly on the map. This is a "necessary" preliminary function to accomplish #73. I can correct all the revisions requested (thanks for looking into it) if that makes sense to you.

I could also implement the cropping by units, which is currently not implemented.

ericpre commented 1 year ago

Okay, it makes more sense. What would be the corresponding workflow in a few line of code?

jordiferrero commented 1 year ago

Okay, it makes more sense. What would be the corresponding workflow in a few line of code?

I am not 100% sure yet but something like this:

def transform_image_to_s_nav_axis(image, s, plot=True):
    read metadata from s on crop + rebinning
    apply crop + rebin to image
    return image_transform & plot with navigator as image

I am not sure if there is a way to store image in the s object and always call it as navigation axis when plotting the signal_1d/2d object s.

ericpre commented 1 year ago

I am not 100% sure yet but something like this:

def transform_image_to_s_nav_axis(image, s, plot=True):
    read metadata from s on crop + rebinning
    apply crop + rebin to image
    return image_transform & plot with navigator as image

Would it make sense to pass the other signal at the same time? Pass a list of signal and all apply the cropping to all of them instead of calling the crop_edge and relying on the metadata?

I am not sure if there is a way to store image in the s object and always call it as navigation axis when plotting the signal_1d/2d object s.

There is lazy signal and it would be good to do it with non-lazy signal.

jordiferrero commented 1 year ago

I am not 100% sure yet but something like this:

def transform_image_to_s_nav_axis(image, s, plot=True):
    read metadata from s on crop + rebinning
    apply crop + rebin to image
    return image_transform & plot with navigator as image

Would it make sense to pass the other signal at the same time? Pass a list of signal and all apply the cropping to all of them instead of calling the crop_edge and relying on the metadata?

I am not sure if there is a way to store image in the s object and always call it as navigation axis when plotting the signal_1d/2d object s.

There is lazy signal and it would be good to do it with non-lazy signal.

Based on this feedback, I think the crop_edges function should be expanded to accepting a list of hyperspy signals. Then it should crop all the signals equally and return them in the same order as input. This means that all the functionalities that crop_edges has now are good. All I need to implement is the multiple signals input. Then this "wrapper for inav" makes more sense, in my opinion. I will implement this and let you review again. Thanks

jordiferrero commented 1 year ago

I am not 100% sure yet but something like this:

def transform_image_to_s_nav_axis(image, s, plot=True):
    read metadata from s on crop + rebinning
    apply crop + rebin to image
    return image_transform & plot with navigator as image

Would it make sense to pass the other signal at the same time? Pass a list of signal and all apply the cropping to all of them instead of calling the crop_edge and relying on the metadata?

I am not sure if there is a way to store image in the s object and always call it as navigation axis when plotting the signal_1d/2d object s.

There is lazy signal and it would be good to do it with non-lazy signal.

Based on this feedback, I think the crop_edges function should be expanded to accepting a list of hyperspy signals. Then it should crop all the signals equally and return them in the same order as input. This means that all the functionalities that crop_edges has now are good. All I need to implement is the multiple signals input. Then this "wrapper for inav" makes more sense, in my opinion. I will implement this and let you review again. Thanks

This is now ready for a final review. All tests and coverage had been sorted out @jlaehne @ericpre

jlaehne commented 1 year ago

The operation on a list is definitely an added value! Did some basic trials on some of my data and it worked nicely.

jlaehne commented 1 year ago

Thanks @jordiferrero for bringing up the issue that some of the filenames and function placements are maybe not so logical. However, moving functions and related tests other than crop_edges should normally be done in a separate PR. Makes it quite complicated to review this PR now.

jordiferrero commented 1 year ago

Thanks @jordiferrero for bringing up the issue that some of the filenames and function placements are maybe not so logical. However, moving functions and related tests other than crop_edges should normally be done in a separate PR. Makes it quite complicated to review this PR now.

@jlaehne You are right. This PR has got out of hand... it's OK. I will open a new PR on this feature (and only this feature) and will close this one in due time. For now, please don't bother reviewing it. Sorry :-)

jlaehne commented 1 year ago

I think you could also consolidate this one and move the stuff concerning other functions to a separate PR. But whatever works better for you :-)

jlaehne commented 1 year ago

continued in #177