-
Notifications
You must be signed in to change notification settings - Fork 39
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
Design tool tweaks #386
Design tool tweaks #386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small notes, some of which (like logging) we can update during the toast3 port.
src/libtoast/src/toast_atm.cpp
Outdated
@@ -35,7 +35,7 @@ bool toast::atm_verbose() { | |||
// First time we were called | |||
auto & env = toast::Environment::get(); | |||
std::string logval = env.log_level(); | |||
if (strncmp(logval.c_str(), "VERBOSE", 7) == 0) { | |||
if (strncmp(logval.c_str(), "DEBUG", 5) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atmosphere simulation code was actually the driving force behind creating a new log-level (VERBOSE) that was even more verbose than the debug level. In the current code, at the VERBOSE level, there are extensive string operations and diagnostic files written to disk from every process. This proposed change would enable all of that output even at DEBUG level, which is a level that is used much more often for general debugging. Is there a particular kind of output you want to dump by this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Makes sense. I restored the original log level.
@@ -178,6 +178,9 @@ def __init__(self, shape, dtype, comm): | |||
|
|||
def __del__(self): | |||
self.close() | |||
# Free the node communicator | |||
if self._nodecomm is not None: | |||
self._nodecomm.Free() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in toast3 we are using the new upstream package of pshmem
. I just opened a matching PR in that repo: tskisner/pshmem#6
proj = np.zeros([nmode, nsample]) | ||
norms = np.zeros(nmode) | ||
|
||
t1 = time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously one could use Timer.start()
, Timer.stop()
, Timer.clear()
, Timer.seconds()
throughout, but as you wish...
templatesT = mask * templates # nmode * ndet | ||
invcov = np.dot(templatesT, templates.T) | ||
try: | ||
cov = np.linalg.inv(invcov) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine that flagging the inverse template covariance for every timesample and inverting it is an expensive part of this operation. I notice that np.linalg.inv()
accepts multiple matrices, but that would require building that list first. I guess one could move the building of the coefficients into compiled code and have each process call that for a range of samples. We could always do that if this is a bottleneck.
src/toast/todmap/groundfilter.py
Outdated
|
||
return coeff | ||
|
||
@function_timer | ||
def subtract_templates(self, ref, good, coeff, cheby_trend, cheby_filter): | ||
def subtract_templates(self, ref, good, coeff, legendre_trend,legendre_filter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the format_source.sh
script run?
src/toast/todmap/groundfilter.py
Outdated
tod = obs["tod"] | ||
if (self.rank == 0 and self.verbose) or (self.grank == 0 and self.verbose > 1): | ||
print("{:4} : OpGroundFilter: Processing observation {} / {}".format( | ||
self.group, iobs + 1, len(data.obs)), flush=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe log at the DEBUG or VERBOSE level? Feel free to save that for the porting work though.
src/toast/todmap/groundfilter.py
Outdated
templates, legendre_trend, legendre_filter = self.build_templates(tod, obs) | ||
if self.grank == 0 and self.verbose > 1: | ||
print("{:4} : OpGroundFilter: Built templates in {:.1f}s".format( | ||
self.group, time() - t1), flush=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you log at debug or verbose level, then no need to have (and check) a separate self.verbose
member.
src/toast/todmap/groundfilter.py
Outdated
if self.rank == 0: | ||
print("Applied ground filter in {:.1f} s. Average rcond of template matrix was {}".format( | ||
time() - t0, self.rcondsum / (self.nsingular + self.ngood) | ||
), flush=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good INFO level message.
src/toast/todmap/filterbin.py
Outdated
template_covariance, | ||
) | ||
if self.grank == 0: | ||
print("{:4} : OpFilterBin: Accumulated in {:.3f}s".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think group-level messages could be logged at DEBUG level. Usually INFO level messages would only be dumped from one process.
Unit tests work again. Merging this branch approved branch. |
Small optimizations in the ground simulation facilities made during the CMB-S4 2021 design tool simulation campaign.