Gp/fix/act flags #947
Gp/fix/act flags #947
Conversation
…rs to make maps with downsampling
|
I think we need to rethink this -- the additions actually break the abstraction (note the SignalCut inherits from Signal); MLMapmaker caches several Signal objects in So I suspect the right thing to do is have the |
|
I moved the path information in the |
|
I think going through the Signal objects is fine. We could also go through the init of the MLMapmaker object and pass it there as a "global" parameter that applies to all sub-signal objects included. |
Included. Also the ACT tod that was causing errors now works. |
sotodlib/mapmaking/utilities.py
Outdated
| - sep: separator. Defaults to `.` | ||
| """ | ||
|
|
||
| flags = aman.copy() |
sotodlib/mapmaking/ml_mapmaker.py
Outdated
| @@ -161,7 +170,8 @@ def __init__(self, name, ofmt, output, ext): | |||
| self.ext = ext | |||
| self.dof = None | |||
| self.ready = False | |||
| def add_obs(self, id, obs, nmat, Nd): pass | |||
| self.glitch_flags = glitch_flags | |||
| def add_obs(self, id, obs, nmat, Nd, glitch_flags:Optional[str]): pass | |||
There was a problem hiding this comment.
Same question.
Can it be replaced with a generic **kwargs, to imply "yes sometimes a subclass will accept and process other random args."
sotodlib/mapmaking/ml_mapmaker.py
Outdated
| @@ -361,9 +373,9 @@ def transeval(self, id, obs, other, map, tod): | |||
|
|
|||
| class SignalCut(Signal): | |||
| def __init__(self, comm, name="cut", ofmt="{name}_{rank:02}", dtype=np.float32, | |||
| output=False, cut_type=None): | |||
| output=False, cut_type=None, glitch_flags:str ="flags.glitch_flags"): | |||
There was a problem hiding this comment.
I would prefer these default to None, as they do in SignalMap. Then convert None -> flags.glitch_flags, to handle the default.
When you have cascading kwargs, e.g.:
SignalMap(..., glitch_flags=None)
-> Signal(glitch_flags=glitch_flags)
-> self.glitch_flags = glitch_flags
it tends to defeat the use of default value declarations for optional parameters. My practice has been to prefer this format for setting default args:
class Signal:
def __init__(self, ..., glitch_flags=None):
if glitch_flags is None:
glitch_flags = 'flags.glitch_flags'
Then subclasses, or whatever, can pass in their default value of None and it propagates cleanly all the way down to whatever base function is comfortable with setting the default value.
However... this isn't necessary relevant if we take glitch_flags out of the base class Signal. In that case you won't have cascading. I still think it's a good practice, in code that might become cascade, to use "None" to represent "the default value specified by some lower level". (Though this makes it harder to use "None" to represent "disable this feature" -- I would normally use False to signal that in cases where it is needed.)
There was a problem hiding this comment.
Generally, I prefer having the actual default value over a check and set in the code. It makes it very easy know the default value, and it avoids any question as to what is happening. I agree the base class needs to have a default as None. Using kargs will resolve that and make it much cleaner for sure.
|
@mhasself let's move forward with this PR. ML mapmaker's multipass is not yet used from |
sotodlib/core/context.py
Outdated
| @@ -5,12 +5,15 @@ | |||
| import logging | |||
| import numpy as np | |||
|
|
|||
| from typing import Union, Dict, Tuple, List | |||
There was a problem hiding this comment.
The changes to context.py are just noise; please revert.
There was a problem hiding this comment.
I will. I put them when I was trying to understand what was happening.
|
|
||
| def setUp(self): | ||
| # Mock objects for testing | ||
| obj1 = MagicMock() |
There was a problem hiding this comment.
Why mocks for these tests? You're not going to test on a real AxisManager even once?
There was a problem hiding this comment.
It is an real Axismanager. Lines 20 to 25 make sure the first high level object is an AxisManager.
Oh... Lines 12-18 are not needed. Sorry I forgot to remove them
There was a problem hiding this comment.
This is for testing with an arbitrary object that has a dict representation and it is accessed from test_parameter_exists_in_dict.
test_parameter_exists_in_fields tests with an AxisManager object:
flags = MagicMock(spec=AxisManager)
flags._fields = {"glitch_flags": "some_value"}
aman = MagicMock(spec=AxisManager)
aman._fields = {"flags": flags,
"other_key": "another_value"}I am not sure it makes sense to create a whole AxisManager object. I can give it a try if you think it is necessary
|
I will also fix the conflict with master and sync again. |
| @@ -1,6 +1,5 @@ | |||
| import so3g.proj | |||
| import numpy as np | |||
| import scipy | |||
There was a problem hiding this comment.
This was not used anywhere in this file.
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| """ | |||
|
|
|||
| from spt3g import core | |||
| from so3g.spt3g import core | |||
| from threadpoolctl import threadpool_limitse | ||
| from threadpoolctl import threadpool_limits |
| pmat = coords.pmat.P.for_tod(obs, comps=comps, geom=(map.shape, map.wcs), rot=rot, threads="domdir", interpol=self.interpol) | ||
| pmat = coords.pmat.P.for_tod(obs, comps=comps, geom=(map.shape, map.wcs), rot=rot, threads="domdir", interpol=interpol) | ||
| # And perform the actual injection | ||
| pmat.from_map(map.extract(shape, wcs), dest=obs.signal) | ||
| pmat.from_map(map.extract(map.shape, map.wcs), dest=obs.signal) |
There was a problem hiding this comment.
@chervias git blame said you are the one that added this lines. I corrected based on my understanding, but can you let me know if this is correct?
This PR makes the glitch path configurable for glitch flags. It assumes that the flags are under a nested path and traverses it based on a separator. This can later be expanded to accept operations over paths.