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.

Sign up for GitHub

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

Jump to bottom

Merlin: try ScanX and ScanY from the HDR file #1631

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

Merlin: try ScanX and ScanY from the HDR file #1631

wants to merge 3 commits into from
+9 −0

Conversation

sk1p
Copy link
Member

@sk1p sk1p commented Apr 25, 2024

Fixes #1630

Contributor Checklist:

Reviewer Checklist:

  • /azp run libertem.libertem-data passed
  • No import of GPL code from MIT code

@sk1p sk1p added this to the 0.15 milestone Apr 25, 2024
@sk1p
Copy link
Member Author

sk1p commented Apr 25, 2024

/azp run libertem.libertem-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 25, 2024

/azp run libertem.libertem-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov Codecov
Copy link

codecov bot commented Apr 25, 2024 •
edited

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 65.79%. Comparing base (7274ed8) to head (fb36c43).

❗ Current head fb36c43 differs from pull request most recent head 54513aa. Consider uploading reports for the commit 54513aa to get more accurate results

Files Patch % Lines
src/libertem/io/dataset/mib.py 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1631      +/-   ##
==========================================
- Coverage   70.02%   65.79%   -4.23%     
==========================================
  Files         325      325              
  Lines       27255    27257       +2     
  Branches     3139     3140       +1     
==========================================
- Hits        19084    17933    -1151     
- Misses       7615     8817    +1202     
+ Partials      556      507      -49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matbryan52
matbryan52 reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@matbryan52 matbryan52 left a comment

Choose a reason for hiding this comment

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

Looks good. Might be good to add a changelog entry to mention that this is now supported?

I will ask if there are any small data sets available with this metadata.

@sk1p
Copy link
Member Author

sk1p commented Apr 29, 2024

Looks good. Might be good to add a changelog entry to mention that this is now supported?

Thanks for having a look! Changelog entry added.

I will ask if there are any small data sets available with this metadata.

Thanks, that would be great 👍 If not, I'll add some unit tests instead that exercise the added logic.

@sk1p
Copy link
Member Author

sk1p commented Apr 29, 2024

Test failure looks spurious, something to fix after the 0.14 release probably:

tests/executor/test_delayed.py:432: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/api.py:988: in run_udf
    return self._run_sync(
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/api.py:1281: in _run_sync
    udf_results = run_gen_get_last(_run_sync_wrap())
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/common/async_utils.py:98: in run_gen_get_last
    result = gen.__next__()
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/api.py:91: in __next__
    return next(self._task_results)
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/api.py:1266: in _inner
    for udf_results in result_iter:
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/udf/base.py:87: in __next__
    return next(self._gen)
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/udf/base.py:[263](https://github.com/LiberTEM/LiberTEM/actions/runs/8877972975/job/24372655636?pr=1631#step:6:264)2: in _inner
    self._apply_part_result(
.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/executor/delayed.py:141: in _apply_part_result
    getattr(dest, k)[:] = getattr(merged, k)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <libertem.executor.utils.dask_inplace.DaskInplaceWrapper object at 0x7fe71c7c73e0>
key = slice(None, None, None)
value = dask.array<from-value, shape=(1,), dtype=float32, chunksize=(1,), chunktype=numpy.ndarray>

    def __setitem__(self, key, value: Union[nt.ArrayLike, Number]):
        if not np.isscalar(value) and value.size == 1:
            # Avoids deprecated Numpy behaviour of implicitly
            # extracting a scalar from an array during assignment
>           value = value.item()
E           AttributeError: 'Array' object has no attribute 'item'

.tox/py3.12/.venv/lib/python3.12/site-packages/libertem/executor/utils/dask_inplace.py:89: AttributeError

@sk1p sk1p added enhancement New feature or request file formats and I/O labels May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matbryan52 matbryan52 matbryan52 left review comments

Assignees
No one assigned
Labels
enhancement New feature or request file formats and I/O
Projects
None yet
Milestone
0.15
Development

Successfully merging this pull request may close these issues.

Merlin: read ScanX and ScanY from the acquisition header (.hdr) if available
2 participants
@sk1p @matbryan52