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

Map Clean Up #3198

Merged
merged 23 commits into from Sep 7, 2023
Merged

Conversation

CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented Jul 20, 2023

Description of the change

This removes the parallel argument for the map function.

  • Removes the parallel argument
  • Rename max_workers to num_workers to be consistent with dask
  • Adds more documentation on setting the dask backend
  • Adds navigation_chunk argument for setting the chunks with a non-lazy signal
  • Fix axes handling when the function to be mapped can be applied to the whole dataset (_map_all code path) - typically when it has the axis or axes keyword argument and the function argument are not navigation coordinate dependent.

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

@CSSFrancis CSSFrancis mentioned this pull request Jul 20, 2023
57 tasks
@CSSFrancis CSSFrancis added this to the v2.0 Split milestone Jul 20, 2023
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 93.43% and project coverage change: -0.01% ⚠️

Comparison is base (56a5db3) 81.30% compared to head (1b2f0cc) 81.30%.
Report is 83 commits behind head on RELEASE_next_major.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3198      +/-   ##
======================================================
- Coverage               81.30%   81.30%   -0.01%     
======================================================
  Files                     176      173       -3     
  Lines                   24406    24207     -199     
  Branches                 5681     5626      -55     
======================================================
- Hits                    19843    19681     -162     
+ Misses                   3258     3224      -34     
+ Partials                 1305     1302       -3     
Files Changed Coverage Δ
hyperspy/_lazy_signals.py 92.30% <ø> (-0.55%) ⬇️
hyperspy/datasets/example_signals.py 100.00% <ø> (ø)
hyperspy/defaults_parser.py 68.25% <ø> (-0.25%) ⬇️
hyperspy/misc/example_signals_loading.py 100.00% <ø> (ø)
hyperspy/signals.py 100.00% <ø> (ø)
hyperspy/drawing/_widgets/horizontal_line.py 86.95% <66.66%> (-3.05%) ⬇️
hyperspy/drawing/_widgets/rectangles.py 41.24% <66.66%> (+0.65%) ⬆️
hyperspy/drawing/_widgets/vertical_line.py 91.30% <66.66%> (+1.30%) ⬆️
hyperspy/drawing/widget.py 73.19% <80.00%> (+2.80%) ⬆️
hyperspy/signal.py 77.58% <81.81%> (-0.15%) ⬇️
... and 10 more

... and 3 files with indirect coverage changes

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

@ericpre ericpre force-pushed the map_clean_up branch 2 times, most recently from d7071fe to 42fe611 Compare September 2, 2023 10:38
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.

@CSSFrancis, I initially wanted to rebase to review and finish this PR, but got sidetracked with the documentation and spotted a bug with _map_all. Can you please have a quick look to check that my changes are sensible?
Other than that it looks good to me.


Running computation on remote cluster can be done easily using ``dask_jobqueue``

.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

Move this example here, the example of the gallery needs to be run and I am not that it would be possible/easy on CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I think maybe the distributed workflow would run but not the slurm workflow so you are right we should move it.

doc/user_guide/signal2d.rst Outdated Show resolved Hide resolved
@ericpre ericpre removed the run-extension-tests Run extension test suites label Sep 2, 2023
@CSSFrancis
Copy link
Member Author

@ericpre I'll look at this a little later today or tomorrow. I've been pretty busy the last week or so and haven't been on top of some of these PRs. :)

I'll have some time the next month to help with the major release.

# Conflicts:
#	doc/user_guide/signal2d.rst
#	examples/lazy/distributed_backend.py
#	hyperspy/_signals/complex_signal.py
#	hyperspy/_signals/hologram_image.py
#	hyperspy/_signals/signal1d.py
#	hyperspy/_signals/signal2d.py
#	hyperspy/signal.py
#	upcoming_changes/3198.api.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.

@CSSFrancis, are you happy with my changes to this PR? If so, can you please merge it?

@CSSFrancis
Copy link
Member Author

@ericpre I'm going to check that the documentation builds this time and then I can merge this. Your changes look good to me!

@CSSFrancis CSSFrancis merged commit b22f2a5 into hyperspy:RELEASE_next_major Sep 7, 2023
21 of 22 checks passed
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.

None yet

2 participants