Open
Conversation
…lass) - the `get_section` method in now an empty method from `BaseWindow` class and is properly implemented for inherited class - when adding window with `add_ell_cl` the duplication of windows with corresponding window indices is also done for TopHatWindow and its inherited class such as `LogTopHatWindow`
joezuntz
requested changes
Apr 11, 2023
Collaborator
joezuntz
left a comment
joezuntz
left a comment
There was a problem hiding this comment.
This looked okay, apart from the note below.
But I'm not committing to this use of the Window functions. I think we need to decide whether to rethink either the basic window functions or the bandpower ones.
| if isinstance(window, BandpowerWindow): | ||
| if len(ell) != window.nv: | ||
| if isinstance(window, (BandpowerWindow, TopHatWindow)): | ||
| nv = window.nv if isinstance(window, BandpowerWindow) else len(window.min) |
Collaborator
There was a problem hiding this comment.
I think this will crash if window.min is an integer?
Contributor
Author
There was a problem hiding this comment.
True, we must ensure window.min is an array. This will fail here but also here.
The min/max class attributes must be arrays. We can use np.asarray within the __init__ of TopHatWindow but I think this is related to the redesign/rethink of the window classes you mention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following discussion #81 , this PR proposes to extend the ability to get window for
(Log)TopHatWindowthe same asBandpowerWindow.When filling data via
add_ell_clfunction, the(Log)TopHatWindowobject is associated to awindow_indtag which indexes its position related to data index.When retrieving the windows via
get_bandpower_windows(it may be worth renaming this function intoget_windowssince it is not anymore dedicated to onlyBandpowerWindow), then the full binning array is computed given the selected indices (here again, in the same way asBandpowerWindow).The important thing to think about is that
(Log)TopHatWindowobject is now storing the whole bin edges and not a pair of min/max value. Nevertheless, it should be backward compatible since min/max scalar values can also be represented by one entry array.We may understand this feature is not required or desired and that it exists other way to extract the windows (using
get_tagfunctions for instance, see #81 discussion) but the important point is that it allows to treatBandpowerWindowand(Log)TopHatWindowobjects the same way without assuming or accessing the type of window.This will close #81