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

Add forward and adjoint gradient operators for regularization, and 2d USFFT functions #82

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nikitinvv
Copy link
Contributor

@nikitinvv nikitinvv commented Jul 12, 2020

Purpose

Add implementation of the forward gradient operator and its adjoint (negative divergence), and 2D usfft functions.

Approach

The operators were implemented by taking differences between neighboring elements for x,y,z dimensions. They can be used inside joint iterative schemes with several subproblems, where one of the subproblems is regularization given in terms of the TV or Tikhonov terms.

2d USFFT functions are the same as for 3D except for the dimensions change and one constant. It is worth to merge them into one USFFT function.

Pre-Merge Checklists

Submitter

  • Write a helpfully descriptive pull request title.
  • Organize changes into logically grouped commits with descriptive commit messages.
  • Document all new functions.
  • Write tests for new functions or explain why they are not needed.
  • Build the documentation successfully
  • Use yapf to format python code.

Reviewer

  • Actually read all of the code.
  • Run the new code yourself.
  • Write a summary of the changes as you understand them.
  • Thank the submitter.

@pep8speaks
Copy link

pep8speaks commented Jul 12, 2020

Hello @nikitinvv! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 16:1: E302 expected 2 blank lines, found 1
Line 43:1: E302 expected 2 blank lines, found 1
Line 47:15: E741 ambiguous variable name 'l'
Line 129:1: E302 expected 2 blank lines, found 1
Line 228:1: E302 expected 2 blank lines, found 1
Line 232:15: E741 ambiguous variable name 'l'
Line 245:17: E131 continuation line unaligned for hanging indent

Comment last updated at 2020-08-13 17:32:11 UTC

@nikitinvv nikitinvv changed the title Add forward and adjoint gradient operators for regularization Add forward and adjoint gradient operators for regularization, and 2d USFFT functions Aug 13, 2020
@nikitinvv
Copy link
Contributor Author

@slnsrydn @carterbox could some of you merge 2d USFFT and 3d USFFT into 1 function?

@carterbox
Copy link
Contributor

Is an ND USFFT separable like the an ND FFT is separable into the product of 1D FFTs or whatever?

Copy link

@slnsrydn slnsrydn left a comment

Choose a reason for hiding this comment

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

thanks

Copy link
Contributor

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

The regularization part of this PR has no tests and is out of date now that there are no NumPy Operators. I'm also not seeing any tests for the new 2D USFFT function though it seems like it is probably correct?

@nikitinvv
Copy link
Contributor Author

@carterbox, I just added the regularization operators that should be used in ptycho-tomo admm. They can be tested only when the admm is ready. Yes, Nd usfft can be done as separate 1d usfft, but on GPU it will be faster to do Nd FFT once than 1D FFT many times. As I see, in the current code it is enough to control only the number of inner loops in the scatter and gather operators, depending on the number of usfft dimensions. It can be done by recursive function calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants