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

Utility functions for iterative algorithms #60

Merged
merged 15 commits into from Nov 12, 2021

Conversation

uellue
Copy link
Member

@uellue uellue commented Sep 23, 2021

  • Fast image transforms using a sparse matrix product
  • Coordinate transforms to match detector data with forward model
  • Calculate stacks of shifted and rolled products of a smaller illumination within a larger object, including sub-pixel shifting
  • Aggregate shifted and rolled updates within a larger object

The functions can perform FFT shifts on the fly to reduce memory transfers. Both CPU and CUDA are supported. They are tested to conform precisely to the conventions of libertem.api.create_com_analysis() and also SSB_UDF.

CC @bangunarya @Sniper2k that's the stuff I have mentioned recently. If you'd like to try it, let me know! :-)

Contributor Checklist:

Intended for extracting data in such a way that it matches the reconstruction grid.

This is a performance-critical and error-prone step in iterative ptychography algorithms.
* Image and coordinate transformations
* Shifted, cropped and rolled products and aggregation
@uellue uellue added the enhancement New feature or request label Sep 23, 2021
@uellue uellue added this to the 0.2 milestone Sep 23, 2021
Co-authored-by: Dieter Weber <d.weber@fz-juelich.de>
@SimeonEhrig
Copy link
Contributor

@uellue Is there are a rendered version of the sphinx doc available? In the alpaka-group, we use readthedoc. There is a CI job, which build the sphinx doc and provides an link to the result.

@uellue
Copy link
Member Author

uellue commented Sep 29, 2021

@uellue Is there are a rendered version of the sphinx doc available? In the alpaka-group, we use readthedoc. There is a CI job, which build the sphinx doc and provides an link to the result.

At the moment the rendered documentation is only available after merging or by running tox -e docs locally. @sk1p, having the rendered docs of a PR available could help with reviewing, since sometimes it builds cleanly but looks terrible. I've reviewed the rendered docs locally up to now, which is a bit tedious.

@SimeonEhrig
Copy link
Contributor

At the moment the rendered documentation is only available after merging or by running tox -e docs locally. @sk1p, having the rendered docs of a PR available could help with reviewing, since sometimes it builds cleanly but looks terrible. I've reviewed the rendered docs locally up to now, which is a bit tedious.

Thanks. I was not sure, if the CI job provides an artifact of the build. But building locally is also fine.

* Shift real and imaginary parts separately to support complex illumination
* Iterate only over shape of updates, not the whole object, to improve
  scaling behavior with large objects and small updates. Use atomic add
  to avoid conflict between threads and blocks writing to the same memory.
Copy link
Contributor

@sk1p sk1p left a comment

Choose a reason for hiding this comment

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

I've added some comments and typo fixes. Maybe we can also make some diagrams for describing the coordinate systems involved, and how the transformations work?

Thanks!

src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
src/ptychography40/reconstruction/common.py Outdated Show resolved Hide resolved
docs/source/algorithms.rst Outdated Show resolved Hide resolved
docs/source/algorithms.rst Outdated Show resolved Hide resolved
uellue and others added 2 commits October 25, 2021 16:59
Co-authored-by: Alexander Clausen <alex@gc-web.de>
Co-authored-by: Alexander Clausen <alex@gc-web.de>
@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #60 (ef0c3e6) into master (fec553e) will decrease coverage by 4.15%.
The diff coverage is 53.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
- Coverage   64.08%   59.92%   -4.16%     
==========================================
  Files           8        8              
  Lines         323      549     +226     
  Branches       38       66      +28     
==========================================
+ Hits          207      329     +122     
- Misses        104      206     +102     
- Partials       12       14       +2     
Impacted Files Coverage Δ
src/ptychography40/reconstruction/common.py 54.54% <53.98%> (-2.13%) ⬇️

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 fec553e...ef0c3e6. Read the comment docs.

Co-authored-by: Alexander Clausen <alex@gc-web.de>
@sk1p
Copy link
Contributor

sk1p commented Oct 27, 2021

It looks like the current mac os x image doesn't include Python 3.6 anymore - we may need to switch to an older one (at least for 3.6). See here: https://github.com/actions/virtual-environments/blob/main/images/macos/toolsets/toolset-10.14.json https://github.com/actions/virtual-environments/blob/main/images/macos/toolsets/toolset-11.json

Thx @sk1p!

* Example notebook with more detail
* Coordinate transform is always a factory function, no need for functools
* Also turn (i)fftshift_coords() into higher-order functions
* Clarify and tweak example
* Docs updates for clarity
@uellue
Copy link
Member Author

uellue commented Oct 29, 2021

It looks like the current mac os x image doesn't include Python 3.6 anymore - we may need to switch to an older one (at least for 3.6). See here: https://github.com/actions/virtual-environments/blob/main/images/macos/toolsets/toolset-10.14.json https://github.com/actions/virtual-environments/blob/main/images/macos/toolsets/toolset-11.json

Should I do sth about that in this PR? Can you give me a pointer?

@uellue uellue requested a review from sk1p October 29, 2021 09:02
@sk1p
Copy link
Contributor

sk1p commented Oct 29, 2021

Should I do sth about that in this PR? Can you give me a pointer?

You could try to set the vmImage in the mac os x matrix to macos-10.14 for the Python36 entry here:

strategy:
matrix:
Python36:
python.version: '3.6'
TOXENV: 'py36'
Python37:
python.version: '3.7'
TOXENV: 'py37'
Python38:
python.version: '3.8'
TOXENV: 'py38'

Should be possible to just specify vmImage as sibling of TOXENV and python.version, like this example.

@uellue uellue mentioned this pull request Oct 29, 2021
@sk1p
Copy link
Contributor

sk1p commented Oct 29, 2021

See also LiberTEM/LiberTEM#1142

@sk1p
Copy link
Contributor

sk1p commented Oct 29, 2021

Hm, that didn't work - still showing "Image: macOS-latest"...

@uellue
Copy link
Member Author

uellue commented Nov 12, 2021

@sk1p is the notebook that shows the transforms understandable? Do we need more documentation, are there other things to address? It would be nice to merge this some time next week to make it easier to use in downstream code. :-)

@sk1p
Copy link
Contributor

sk1p commented Nov 12, 2021

is the notebook that shows the transforms understandable?

Yes, thanks!

Do we need more documentation, are there other things to address? It would be nice to merge this some time next week to make it easier to use in downstream code. :-)

I think documentation can always be improved, but let's merge this now (perfect is the enemy of good, and all that...)

@sk1p sk1p merged commit db04eee into Ptychography-4-0:master Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants