WIP: `FieldZeroPadder`: enable padding at the start of the array
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.
Merge request reports
Activity
added 7 commits
-
a6410316...2cdf7973 - 6 commits from branch
NIFTy_6
- 6968fc43 - Merge branch 'NIFTy_6' into options_for_zeropadder
-
a6410316...2cdf7973 - 6 commits from branch
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): changed this line in version 3 of the diff
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.added 9 commits
-
6968fc43...ccbf5012 - 7 commits from branch
NIFTy_6
- b3a065ec - Merge branch 'NIFTy_6' into options_for_zeropadder
- abc3c635 - simplify option handling
-
6968fc43...ccbf5012 - 7 commits from branch
marked as a Work In Progress from 7a2fc19f
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!
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.
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?
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...
mentioned in merge request !612
I am actually still using
NIFTy_6
because of the compatibility-breaking changes to theCorrelatedFieldMaker.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.