Conversation
luojing1211
requested review from
aarchiba,
demorest,
mtlam,
paulray and
scottransom
luojing1211
force-pushed
the
refact_component_type
branch
from
c7c07bc to
8f37f0b
Compare
|
Hi all, could you please take a look at this PR. I would like to move it forward for the wideband TOA development. |
| sector = DelaySector(self.all_components["AstrometryEquatorial"]()) | ||
| copy_sector = deepcopy(sector) | ||
|
|
||
| assert id(sector) != id(copy_sector) |
There was a problem hiding this comment.
What is this testing? Whether copy.deepcopy works?
| assert sector.__class__.__name__ == "DelaySector" | ||
| assert len(sector.component_list) == 1 | ||
| assert hasattr(sector, "delay") | ||
| assert hasattr(sector, "delay_funcs") |
There was a problem hiding this comment.
I'm not really clear what the sector functions do but this seems like it's not testing very much - I'm not clear what aspects it is actually testing?
| assert len(tm.DelayComponent_list) == 4 | ||
| assert len(tm.PhaseComponent_list) == 2 | ||
| assert len(tm.delay_components) == 4 | ||
| assert len(tm.phase_components) == 2 |
There was a problem hiding this comment.
This renaming seems like a definite improvement. But where are the tests of the new functionality coming from this change?
| ---------- | ||
| param: str | ||
| Parameter name | ||
| target_component: str |
There was a problem hiding this comment.
Should this accept a Component object as well as the name of a Component?
| special | ||
| ``` | ||
|
|
||
| ```python |
| """ Class for holding all delay components and their APIs | ||
| """ | ||
|
|
||
| _methods = ( |
There was a problem hiding this comment.
It looks like some of these are methods and others are properties?
| return ft | ||
|
|
||
|
|
||
| def get_component_type(component): |
There was a problem hiding this comment.
Does this return the actual type object? Is it "type" in the python sense you mean here?
| Object --> component --> TypeComponent. (i.e. DelayComponent) | ||
| This class type is in the third to the last of the getmro returned | ||
| result. | ||
|
|
There was a problem hiding this comment.
No need for blank lines at the end of a docstring.
| """ | ||
| for k, v in model.components.items(): | ||
| assert model.get_component_type(v) != v.category | ||
| assert get_component_type(v) != v.category |
There was a problem hiding this comment.
This makes it look like here "type" means the same thing as "category"?
| ) | ||
|
|
||
|
|
||
| class TestSector: |
There was a problem hiding this comment.
It seems worth checking that the code correctly rejects sectors that share "methods", that all listed methods in existing sectors actually exist, and that the "external_sector_map" actually does what it's supposed to.
2 participants
This Pull Request re-facts the way TimingModel stores its components and the APIs to gather the information from the same type of components. The user interface has changed as little as possible.
Motivation:
When developing Wideband TOAs, we are adding several components: DMJumps, DM noises. The current TimingModel is too big and not modulized enough.
Things are changed:
Benefits: