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

Obs matrix #374

Merged
merged 17 commits into from
Feb 2, 2021
Merged

Obs matrix #374

merged 17 commits into from
Feb 2, 2021

Conversation

keskitalo
Copy link
Member

Add a new operator to filter and bin signal: OpFilterBin. It is also able to calculate the matching observation matrix.

@keskitalo
Copy link
Member Author

keskitalo commented Nov 30, 2020

This PR is ready for review and merge. There are features such as caching of intermediate products that we will want to add but they are not critical right now.

Copy link
Member

@tskisner tskisner left a comment

Choose a reason for hiding this comment

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

Impressive new tool! Going to be some work to port to toast3, but good to merge this sooner rather than later so that the work can begin.


void accumulate_observation_matrix(py::array_t <double,
py::array::c_style | py::array::forcecast> c_obs_matrix,
py::array_t <int64_t, py::array::c_style | py::array::forcecast> c_pixels,
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this code in the python bindings, we should collectively come up with some developer documentation about:

  • When should we pass buffers as arguments vs. explicit numpy arrays? Are there performance or other consequences for one choice vs the other?
  • How do we draw the line between calling libtoast functions from the bindings vs just putting that C++ code directly in the bindings? Should we just build the full library here? Should we move the bindings directly into the library?

I don't know the answer to these questions yet, but it does feel like we are using multiple "patterns" to accomplish the same thing throughout the bindings.

from ..todmap import OpFilterBin


def add_filterbin_args(parser):
Copy link
Member

Choose a reason for hiding this comment

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

These options will all become Operator class traits in toast3

from ..timing import function_timer


class OpFilterBin(Operator):
Copy link
Member

Choose a reason for hiding this comment

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

Just some notes here for when we port to toast3... The current BinMap operator (

class BinMap(Operator):
) takes an arbitrary extra operator to apply to the data before making the map. This can be a Pipeline of arbitrary other operators.


@function_timer
def _get_detweights(self, data):
# FIXME : We need a generic way of building the detector weights
Copy link
Member

Choose a reason for hiding this comment

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

Yes, in toast3, every Noise instance has a method to return the detector weights.


phase = self._get_phase(tod)
t1 = time()
common_templates = self._build_common_templates(
Copy link
Member

Choose a reason for hiding this comment

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

More notes for toast3 porting... It seems like the FilterBin operator could be constructed like:

  1. operator which builds common templates for every observation and view
  2. operator which builds and regresses per-detector templates
  3. operator which accumulates observation matrix
  4. Pipeline which runs pointing expansion and (2.) - (3.) one detector at a time

Then the FilterBin operator can internally run (1.) and then BinMap passing the pipeline in (4.) as the pre_process option. Anyway, I'm sure we'll iterate on this along the way.

@keskitalo keskitalo merged commit eb1e650 into master Feb 2, 2021
@tskisner tskisner deleted the obs_matrix branch February 2, 2021 18:58
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.

2 participants