Update metadata used in scale_by_exposure #116

jlaehne · 2022-03-22T21:40:51Z

Description of the change

update scale_by_exposure according to Metadata structure #109
some tests can only run with Hyperspy v1.7 (they work locally with the development branch)
added extra comments in contributors guide

Progress of the PR

Change implemented (can be split into several points),
docstring updated (if appropriate),
added tests (partly still commented out),
activate remaining tests once HyperSpy v1.7 is out,
added line to CHANGELOG.md,
ready for review.

Note

Tests run locally, should work once HyperSpy v1.7 is released. Merge will need to wait for the release, but can be reviewed already for a speedy release of LumiSpy v0.2 after the HyperSpy release.

ericpre · 2022-04-23T10:47:13Z

lumispy/signals/common_luminescence.py

+            if self.metadata.has_item(
+                "Acquisition_instrument.Detector.integration_time"
+            ):
                exposure = float(
-                    self.metadata.get_item("Acquisition_instrument.CL.exposure")
+                    self.metadata.get_item(
+                        "Acquisition_instrument.Detector.integration_time"
+                    )
                )
-            elif self.metadata.has_item("Acquisition_instrument.CL.dwell_time"):
+            # following will work only from hyperspy v1.7
+            elif self.metadata.has_item("integration_time", full_path=False):
                exposure = float(
-                    self.metadata.get_item("Acquisition_instrument.CL.dwell_time")
+                    self.metadata.get_item("integration_time", full_path=False)
                )
+            elif self.metadata.has_item("exposure", full_path=False):
+                exposure = float(self.metadata.get_item("exposure", full_path=False))
+            elif self.metadata.has_item("dwell_time", full_path=False):
+                exposure = float(self.metadata.get_item("dwell_time", full_path=False))


Would it more simple and readable to do something along the line of (assuming the default value of exposure is None, which is more standard than float('nan')):

if exposure is None: # exposure is not provided, try to get it from metadata exposure = self.metadata.get_item("Acquisition_instrument.Detector.integration_time") if exposure is None: # integration_time is not in the metadata, raise an error raise ValueError("Provide `exposure` argument or set `integration_time` in the metadata.")

Why should other cases (integration time saved elsewhere in the metadata) be supported? The point of defining a metadata specification is avoid this kind of thing!

Why should other cases (integration time saved elsewhere in the metadata) be supported? The point of defining a metadata specification is avoid this kind of thing!

Because, even though we have a metadata convention, not all readers parse the metadata yet. But in fact, we should then search the original_metadata and not the metadata.

The readers need to be fixed, then! :)
The motivation for the metadata specification is for user and hyperspy functionalities to know where to find relevant information and avoid using heuristic to find it which in my opinion is a bad practice and it needs to be avoided. I don't think, that this is explained in the hyperspy user guide, which would be good to do.

Considering that lumispy doesn't have a stable API at the moment, I would not worry too much about rough edges like this, where the expose needs to be passed explicitly and favour the use of good practice.

ericpre · 2022-04-23T10:48:15Z

lumispy/signals/common_luminescence.py

-            Exposure time in s. If not given, the function tries to find
-            'exposure' or 'dwell_time' in the metadata (for the moment only at
-            Gatan specific nodes).
+            Exposure time in s. If not given, the function tries to use the


Rename the exposure argument to integration_time so that it is consistent with the metadata specification?

Yes, it would make sense, but I would not want to rename the function not to break the API and would probably allow exposure as backwards compatible Alias.

Added deprecation warning.

codecov-commenter · 2022-04-24T15:53:33Z

Codecov Report

Merging #116 (b09aa33) into main (2258151) will increase coverage by 0.98%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   95.76%   96.75%   +0.98%     
==========================================
  Files          11       11              
  Lines         591      586       -5     
==========================================
+ Hits          566      567       +1     
+ Misses         25       19       -6

Impacted Files	Coverage Δ
lumispy/signals/common_luminescence.py	`100.00% <100.00%> (ø)`
lumispy/utils/axes.py	`98.08% <0.00%> (+1.19%)`	⬆️
lumispy/signals/luminescence_spectrum.py	`91.75% <0.00%> (+1.33%)`	⬆️
lumispy/signals/cl_spectrum.py	`98.46% <0.00%> (+1.44%)`	⬆️

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 2258151...b09aa33. Read the comment docs.

lumispy/signals/common_luminescence.py

ericpre · 2022-04-26T07:01:58Z

lumispy/signals/common_luminescence.py

+                integration_time = kwargs["exposure"]
+                raise DeprecationWarning(
+                    "The `exposure` argument will be "
+                    "removed in LumiSpy 1.0, use `integration_time` instead."


Same as above: better to use rename?

ericpre · 2022-04-26T07:04:23Z

lumispy/signals/common_luminescence.py

+            if self.metadata.has_item(
+                "Acquisition_instrument.Detector.integration_time"
+            ):
                exposure = float(
-                    self.metadata.get_item("Acquisition_instrument.CL.exposure")
+                    self.metadata.get_item(
+                        "Acquisition_instrument.Detector.integration_time"
+                    )
                )
-            elif self.metadata.has_item("Acquisition_instrument.CL.dwell_time"):
+            # following will work only from hyperspy v1.7
+            elif self.metadata.has_item("integration_time", full_path=False):
                exposure = float(
-                    self.metadata.get_item("Acquisition_instrument.CL.dwell_time")
+                    self.metadata.get_item("integration_time", full_path=False)
                )
+            elif self.metadata.has_item("exposure", full_path=False):
+                exposure = float(self.metadata.get_item("exposure", full_path=False))
+            elif self.metadata.has_item("dwell_time", full_path=False):
+                exposure = float(self.metadata.get_item("dwell_time", full_path=False))


The readers need to be fixed, then! :)
The motivation for the metadata specification is for user and hyperspy functionalities to know where to find relevant information and avoid using heuristic to find it which in my opinion is a bad practice and it needs to be avoided. I don't think, that this is explained in the hyperspy user guide, which would be good to do.

Considering that lumispy doesn't have a stable API at the moment, I would not worry too much about rough edges like this, where the expose needs to be passed explicitly and favour the use of good practice.

ericpre

Looks good, I suggested a clearer wording onthe deprecation to avoid confusing about when the deprecation starts and ends and when the exposure will stop working.

lumispy/signals/common_luminescence.py

update scale exposure metadata

f7bac56

jlaehne added this to the v0.2.0 milestone Mar 22, 2022

jlaehne added the status: WIP work in progress label Apr 9, 2022

jlaehne added status: needs review and removed status: WIP work in progress labels Apr 21, 2022

jlaehne mentioned this pull request Apr 21, 2022

LumiSpy v0.2 #121

Closed

ericpre reviewed Apr 23, 2022

View reviewed changes

rename exposure argument

7f2f925

jlaehne added 3 commits April 24, 2022 17:54

black formatting

aac3b7d

uncomment tests and fixes

9f851be

changelog entry

4706596

ericpre reviewed Apr 26, 2022

View reviewed changes

remove search

9b7e816

jlaehne force-pushed the scale-exposure-update branch from b09aa33 to 9b7e816 Compare April 26, 2022 22:25

ericpre approved these changes Apr 27, 2022

View reviewed changes

lumispy/signals/common_luminescence.py Outdated Show resolved Hide resolved

lumispy/signals/common_luminescence.py Outdated Show resolved Hide resolved

jlaehne added 3 commits April 28, 2022 00:49

deprecation wording

a012e1f

fix test

86437ef

fix deprecation warning

3ec819b

jlaehne merged commit 2ea75a0 into LumiSpy:main Apr 27, 2022

jlaehne deleted the scale-exposure-update branch April 27, 2022 23:03

jlaehne removed the status: needs review label Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update metadata used in scale_by_exposure #116

Update metadata used in scale_by_exposure #116

jlaehne commented Mar 22, 2022 •

edited

Loading

ericpre Apr 23, 2022 •

edited

Loading

jlaehne Apr 24, 2022

ericpre Apr 26, 2022

ericpre Apr 23, 2022

jlaehne Apr 24, 2022

jlaehne Apr 24, 2022

codecov-commenter commented Apr 24, 2022 •

edited

Loading

ericpre Apr 26, 2022

ericpre Apr 26, 2022

ericpre left a comment

Update metadata used in scale_by_exposure #116

Update metadata used in scale_by_exposure #116

Conversation

jlaehne commented Mar 22, 2022 • edited Loading

Description of the change

Progress of the PR

Note

ericpre Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

jlaehne Apr 24, 2022

Choose a reason for hiding this comment

ericpre Apr 26, 2022

Choose a reason for hiding this comment

ericpre Apr 23, 2022

Choose a reason for hiding this comment

jlaehne Apr 24, 2022

Choose a reason for hiding this comment

jlaehne Apr 24, 2022

Choose a reason for hiding this comment

codecov-commenter commented Apr 24, 2022 • edited Loading

Codecov Report

ericpre Apr 26, 2022

Choose a reason for hiding this comment

ericpre Apr 26, 2022

Choose a reason for hiding this comment

ericpre left a comment

Choose a reason for hiding this comment

jlaehne commented Mar 22, 2022 •

edited

Loading

ericpre Apr 23, 2022 •

edited

Loading

codecov-commenter commented Apr 24, 2022 •

edited

Loading