Skip to content
Snippets Groups Projects

WIP: `FieldZeroPadder`: enable padding at the start of the array

Closed Lukas Platz requested to merge options_for_zeropadder into NIFTy_6
1 unresolved thread

The FieldZeroPadder was able to pad either in the center of the field arrays or at the end.

Added the option to also pad at the start of field arrays.

Edited by Lukas Platz

Merge request reports

Pipeline #76510 passed

Pipeline passed for e017e4a1 on options_for_zeropadder

Test coverage 86.00% (0.00%) from 1 job

Closed by Lukas PlatzLukas Platz 3 years ago (Mar 24, 2021 11:15am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
42 padding_position : string, optional
43 Where to add the padding. Valid values are 'front', 'center' and 'back'
44 correponding to padding at the beginning of the domain axes, in the
45 middle and at the end of them.
46 central : bool, optional, DEPRECATED
47 `True` is equivalent to ``padding_position = 'center'``.
48 If `False` and `padding_position` is not given, padding is performed
49 at the end of the domain axes.
45 50
46 51 Notes
47 52 -----
48 53 When doing central padding on an axis with an even length, the "central"
49 54 entry should in principle be split up; this is currently not done.
50 55 """
51 def __init__(self, domain, new_shape, space=0, central=False):
56 def __init__(self, domain, new_shape, space=0, padding_position=None, central=None):
  • I must say that I don't feel good about the design of this operator any more ... it does things which look extremely similar, but have totally different meanings in practice.

    Padding at the end is typically done for position-space fields in preparation for a convolution. Padding in the center will only ever be used for fields in harmonic space, to improve resolution in the corresponding position space, correct?

    Independent of all this: wouldn't it be even more general (and still simpler to implement) to make padding_position a tuple of integers which indicate at which indices the zeros should be inserted? Passing zeros here would mean "pad at the front", passing the axis length+1 would mean "pad at the end" etc.

  • Lukas Platz added 9 commits

    added 9 commits

    Compare with previous version

  • Lukas Platz added 1 commit

    added 1 commit

    • 7a2fc19f - fixup! simplify option handling

    Compare with previous version

  • Lukas Platz marked as a Work In Progress from 7a2fc19f

    marked as a Work In Progress from 7a2fc19f

  • Lukas Platz added 1 commit

    added 1 commit

    Compare with previous version

  • Lukas Platz added 1 commit

    added 1 commit

    Compare with previous version

  • Author Maintainer

    Yes, I see your point - that would be much more elegant.

    On the other hand, most use cases will just be padding in the three locations enabled by the interface as of now, so maybe it is alright for the moment.

    If you don't like it in this state, I completely understand and will just make a copy of the operator with the changes in it in my private code. Thank you for the review in any case!

  • Lukas Platz unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Sorry for letting this lie for so long ... didn't really find any time to think about all the potential problems.

    One example is: how does padding in the middle work on a field in the frequency domain when the axis length is even? The docstring currently has the remark

        Notes
        -----
        When doing central padding on an axis with an even length, the "central"
        entry should in principle be split up; this is currently not done.

    but this claim ("it should be split up") is only correct if the field is in the frequency domain. In any case, the design I suggested does not cope with such complications.

    If we want to be able to do all this in a clean way, a single zero-padding operator for both real-space and harmonic space won't work; its interface will be far too messy.

  • Author Maintainer

    I agree that splitting it into two operators would be the clean way to go.

    So, one for padding at arbitrary positions in real space fields and one for central padding of harmonic space fields, right?

    Unfortunately I don't have time to work on it any more. Should I recite this in an issue and close the MR?

  • If that's OK with you, let's be pragmatic and just leave the MR open for now!

    If you need the changes for your own work, would it be acceptable for you to have the modified operator in your own sources for the time being?

  • Author Maintainer

    Sure!

  • Lukas Platz marked as a Work In Progress

    marked as a Work In Progress

  • Lukas Platz changed the description

    changed the description

  • Do we still need this merge request? If so, it needs to be ported to NIFTy_7 (and possibly rebased). If not, I would propose to close it.

  • Not sure. @lplatz, are you still working with NIFTy_6?

    BTW I have been doing zero-padding in other projects in the meantime, and it seems that the Nyquist value in even-length arrays in the Fourier domain doesn't need to be split, but it must rather go entirely into the negative frequency bin. This is all extremely confusing...

  • Lukas Platz mentioned in merge request !612

    mentioned in merge request !612

  • Author Maintainer

    I am actually still using NIFTy_6 because of the compatibility-breaking changes to the CorrelatedFieldMaker.add_fluctuations function. I don't want to repeat a months long prior parameter search - although the new features tempt me constantly …

    In the meantime, I needed the feature again an so wrote the FieldZeroPadder version in !612. The __init__ function is longer and does more safety checks, but the actual application code is much shorter as implemented. I have not compared performance tbh.

    What do you think of it? I will close this merge request - let us continue the discussion on the new one.

  • closed

  • Lukas Platz changed the description

    changed the description

  • Please register or sign in to reply