DM-42870: Add ModelRebuilder #9
Conversation
| def get_is_extended(self) -> np.ndarray: | ||
| return self.table["refExtendedness"] >= 0.5 |
There was a problem hiding this comment.
I'm surprised that you're still using the current extendedness model, or is this a different extendedness column that MPF creates?
There was a problem hiding this comment.
No, it's propagated from the objectTable_tract. If you mean why use that column instead of refSizeExtendedness, I just haven't updated the downstream tasks to do that yet.
| # This is a bit of an ugly hack, refactor this method? | ||
| kwargs_table = {name: getattr(table, name) for name in table.__pydantic_fields__ if name != "table"} | ||
| table_within = type(table)(table=table.table[within], **kwargs_table) |
There was a problem hiding this comment.
Yeah, this is a nice hack but I wonder if just creating an updatedTable method that returns a new instance of the ObjectTable with a new table would be cleaner.
There was a problem hiding this comment.
I think I will make this a method on the base class with these same lines as a default implementation and let subclasses override if the kwargs_table hack doesn't work.
Also it turns out slicing an astropy table makes a copy... always? I can't see a way to get a view. Oh well, it's not important for this ticket.
| fit_results: astropy.table.Table = pydantic.Field(doc="Multiprofit model fit results") | ||
| task_fit: MultiProFitSourceTask = pydantic.Field(doc="The task") | ||
|
|
||
| @cached_property |
There was a problem hiding this comment.
Thanks for introducing me to the @cached_property decorator!
There was a problem hiding this comment.
It's great! It improves readability and performance and makes practically immutable properties. I don't know if I'd be bold enough to use it on non-frozen dataclasses or more easily mutable classes, though.
| parameter values. | ||
| """ | ||
|
|
||
| catexps: list[CatalogExposurePsfs] = pydantic.Field(doc="List of catalog-exposure-psf objects") |
There was a problem hiding this comment.
Maybe add a little more info about what a catalog-exposure-psf object is, perhaps in the docstring?
| # I have no idea what to put for initInputs. | ||
| # quantum.initInputs looks wrong - the values can be lists | ||
| # quantumgraph.initInputRefs(taskDef) returns a list of DatasetRefs... | ||
| # ... but I'm not sure how to map that to connection names? | ||
| task: CoaddMultibandFitTask = taskDef.taskClass(config=config, initInputs={}) |
There was a problem hiding this comment.
Yeah I haven't dived deep enough into the middle ware to know what this does, but I guess you don't need it?
There was a problem hiding this comment.
It works as is, but when I originally looked in to it, I couldn't work out under which circumstances I might add a connection to the task and break this line. I guess we'll find out and I'll leave this comment here to jog my memory if that happens.
| catexps=catexps, task_fit=task_fit, catalog_multi=inputs["cat_ref"], fit_results=cat_output, | ||
| ) | ||
|
|
||
| def make_config_data(self): |
There was a problem hiding this comment.
Please add docstrings for methods in this class.
| offsets[g2f.CentroidXParameterD] = -offset_cen | ||
| offsets[g2f.CentroidYParameterD] = -offset_cen |
There was a problem hiding this comment.
So the models are always square? Is there any chance that will change in the future?
There was a problem hiding this comment.
Ah, no, these aren't bounding box offset but coordinate system offsets. MultiProFit defines the bottom-left corner of the bottom-left pixel as (0,0)* whereas it's (-0.5, -0.5) in the stack. I left that parameter general but I found it even less likely that someone would need to make it asymmetric than to give a value other than 0 or 0.5.
*I might regret this choice in the future but it's a bit late to change now.
| FigureAxes = tuple[Figure, Axes] | ||
|
|
||
|
|
||
| @dataclass(frozen=True, config=pydantic.ConfigDict(arbitrary_types_allowed=True)) |
There was a problem hiding this comment.
Naive question, but what's the gain in using a pydantic dataclass with lots of explicit pydantic code over using pydantic.BaseModel directly? The @dataclass decorator is confusing to me at first glance when this is clearly a data model.
There was a problem hiding this comment.
It's really just force of habit as I started using pydantic dataclasses to replace dataclasses.dataclass, which is what they were made for. I don't think I've ever actually extended BaseModel.
Looking into this there is some discussion of the small differences in behaviour between dataclass and BaseModel here: pydantic/pydantic#710. I don't think any of those issues have impacted me yet, so I'm uncertain as to whether it's best to use one or the other. In the same way that using dataclass over BaseModel looks strange to someone familiar with pydantic, using dataclass should look familiar to anyone who only uses standard library dataclasses (though I suppose by that argument I should use from pydantic import Field rather than pydantic.Field).
There was a problem hiding this comment.
My main comment would be that if you are wanting to switch to pydantic for new code it would be better to use BaseModel -- the ConfigDict you are using is also non-standard for dataclasses. The pydantic dataclasses support is to ease migration of existing code and not something that we should be doing for new code unless we are worried that we might drop pydantic in the future. cc/ @TallJimbo
There was a problem hiding this comment.
Fair enough, I changed to BaseModel and it was painless.
taranu
force-pushed
the
tickets/DM-42870
branch
from
ddddd08
to
831d686
Compare
taranu
force-pushed
the
tickets/DM-42870
branch
from
831d686
to
a2efa25
Compare
No description provided.