DM-53388: Add copy, deepcopy, and getitem methods to most scarlet_lite objects #23
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 92.41% 92.81% +0.40%
==========================================
Files 49 49
Lines 5432 6013 +581
==========================================
+ Hits 5020 5581 +561
- Misses 412 432 +20 ☔ View full report in Codecov by Sentry. |
fred3m
force-pushed
the
tickets/DM-53388
branch
4 times, most recently
from
cb4a3d4 to
5954b92
Compare
python/lsst/scarlet/lite/bbox.py
Outdated
|
|
||
| def __deepcopy__(self, memo: dict[int, Any]) -> Box: | ||
| """Deep copy of the box""" | ||
| if id(self) in memo: |
There was a problem hiding this comment.
I assume this is not performance-critical but you can do if (item := memo.get(id(self))) is not None: return item.
There was a problem hiding this comment.
Yeah, it's not a big deal but I did define my_id = id(self) since it is used multiple times in this method both inside and outside of the if block.
| """Implementation of unused abstract method""" | ||
|
|
||
| def parameterize(self, parameterization: Callable) -> None: | ||
| """Implementation of unused abstract method""" |
There was a problem hiding this comment.
Shouldn't this at least warn, if not raise NotImplementedError, rather than silently do nothing?
There was a problem hiding this comment.
I added a logger warning. There is nothing wrong with having a component that does not update but I agree with your point that the user should at least be warned if they are using it in fitting code.
| return self._model | ||
|
|
||
| def resize(self, model_box: Box) -> bool: | ||
| """Test whether or not the component needs to be resized""" |
There was a problem hiding this comment.
I probably reviewed this ages ago but I think it's more clear to say something like "Resize the component if needed and return whether it was resized".
This maybe should also warn if it can't ever be resized and if that could be surprising to users.
| with self.assertRaises(AssertionError): | ||
| assert_array_equal(param.x, param_deepcopy.x) | ||
|
|
||
| assert_array_equal(param.helpers["m"], param_deepcopy.helpers["m"]) |
There was a problem hiding this comment.
I think this is repetitive enough to warrant a loop.
| self.assertIsNot(source, source_copy) | ||
| self.assertEqual(source.n_components, 2) | ||
| self.assertEqual(source.n_components, source_copy.n_components) | ||
| self.assertFactorizedComponentEqual(source.components[0], source_copy.components[0]) |
fred3m
force-pushed
the
tickets/DM-53388
branch
from
59b01cd to
de53ac2
Compare
2 participants
Checklist
doc/changes