refactor: Pass LUTModel to LUTView #87
Conversation
|
i took the liberty of fixing merge conflicts (which were caused by my docs stuff in #84) |
There was a problem hiding this comment.
this is great @gselzer. I really like the direction this is heading!
There's one important thing that's been lost though (and it looks like I hadn't added a test for this yet) and that is the synchronization between auto-scale and clims:
- manually setting the clims needs to turn off autoscaling
- manually toggling autoscale (when it was previously off) needs to update the clims slider
Screen.Recording.2025-01-12.at.11.23.32.AM.mov
this is a somewhat tricky thing to actually do, because it can cause circular signals (e.g. setting autoscale=True on the model will set clims to the autoscaled version ... but that should not result in model.autoscale=False. By contrast, if the user sets clims (as opposed to the action of autoscale), then we do need to set model.autoscale = False). For example, on main:
Untitled.mov
one thing I fear may happen with this PR is that that complicated behavior now needs to be re-implemented on each frontend view? whereas those set_without_signal methods were previously handling that for all views (but I'm not sure yet)
if it would help, I can try to add this test to main? |
Yeah, good catch, let me try to work this in tomorrow.
Well I hope a lot of the logic can go within the
If you want to, and it's not a lot of effort, then sure! |
This has been fixed with 9abec15.
Yeah, I mean there's a little bit - let me know if you think the changes are not worth it. I think that the code added to jupyter is the worst - if you know of a better way to handle that code, let me know! It's worth noting that for wx, it seems like the default
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 75.76% 76.41% +0.64%
==========================================
Files 49 49
Lines 4952 4956 +4
==========================================
+ Hits 3752 3787 +35
+ Misses 1200 1169 -31 ☔ View full report in Codecov by Sentry. |
Aligns with LutView
Use Optional"[LutModel]" over "LutModel | None" for Python 3.9 Remove unused ctor
Since the actual value to set here is gathered from the data, not from the user, it shouldn't really be on the view. Also will make things much easier when multiple handles have different data.
The use of a controller was causing failures in some places.
|
just took another look here. As mentioned in conversations on zulip, I think the current deal breaker for me here is what this change, as written, implies for what it means to write views. It moves us away from a pattern where all one has to do in order to implement a frontend is simply add a few setter methods for each property, and into a place where each view has to carefully manage their own events, in their own pattern, in order to update the model in the correct way. To me, that opens up both the possibility that different views behave quite differently from each other, and it also puts a bigger burden on what it means to add additional fields to the model, and to add new possible views. It does make the controller now look very nicely slim, but the slimness of that 1 controller comes at the expense of the fatness of all the views. I definitely agree we haven't yet landed on particularly nice pattern in main either, but i also don't think i'd be happier going in this direction. as a side-note: it's conceivable that #94 might have removed the need for all of the blockers that are currently in place (and which you are trying hard to maintain in this PR). The reason would be that it removed the one thing that caused us to need to know who was updating the model (i.e. whether it was the view or the user). With that gone, we may well never have to worry about vicious loops. |
Yeah, that is also the largest drawback for me. It is nice to be able to abstract the different event intricacies behind psygnal.
I don't think I understand what you're worried about here, can you give an example? The only additional burden here is that the LutViews must avoid side-effects, as we cannot rely on psygnal to avoid them. It is a bit more code, but it is also a cleaner contract and simpler control flow. Adding additional fields to the model should be equally difficult either way (although we could make that easier by adding no-op implementations to the view ABC).
I do think that this design simplifies control flow, simplifying debugging and maintenance, but if you feel strongly here then I'm content to move on from this experiment! |
There was a problem hiding this comment.
ok, I think i'm having a change of heart here :)
I sat down to pick out the parts i liked from the parts i didn't, and honestly really wasn't left with too much besides the front-end views, and even those weren't as much as i thought they were. (again, apologies for my waffling on this). I left a suggestion for cleaning up the jupyter pattern, we can likely add one to wx later too)
in addition to the other comments , I think if we added __eq__ operators to ClimPolicy, we would avoid a lot of unnecessary signals (which only get emitted when the value is not equal to the previous value):
class ClimsManual(ClimPolicy):
...
def __eq__(self, other: object) -> bool:
return (
isinstance(other, ClimsManual)
and self.min == other.min
and self.max == other.max
)
class ClimsMinMax(ClimPolicy):
...
def __eq__(self, other: object) -> bool:
return isinstance(other, ClimsMinMax)
class ClimsPercentile(ClimPolicy):
...
def __eq__(self, other: object) -> bool:
return (
isinstance(other, ClimsPercentile)
and self.min_percentile == other.min_percentile
and self.max_percentile == other.max_percentile
)
class ClimsStdDev(ClimPolicy):
...
def __eq__(self, other: object) -> bool:
return (
isinstance(other, ClimsStdDev)
and self.n_stdev == other.n_stdev
and self.center == other.center
)would you mind integrating those and then marking this not-draft when ready? i think it's likely good to go at that point
Oh, yeah, there are some nice ideas here! Let me take a look! |
Will prevent a fair number of signal reemissions. Thanks to @tlambert03 for the suggestion
Thanks again @tlambert03 for the suggestion
tlambert03
changed the title
|
the fails are unrelated... gotta figure out how to get us back to robust CI tests. ... but later. thanks for this! |
This PR is designed to simplify the current MVC architecture of the lookup tables. This work was originally part of a larger PR that does similar things to all of the model-view pairings, but there's no reason it cannot stand on its own and be scrutinized earlier. In other words, it's nice to discuss whether this is a good idea before applying this design to the rest of the codebase.
As noted in #70, the resulting design is simpler, removing signals from the LUTView objects. They instead maintain access to a
LUTModelobject, which is updated directly. Much of the boilerplate logic in theChannelControllerthus goes away, and it simply becomes an organizer for oneLUTModeland manyLUTViews (which in practice is a LUT widget, an image handle, and sometimes a histogram)One additional benefit here is that testing
LUTModelandLUTViewshould become much easier without the controller being needed. I'd like to add some of these tests as a part of this PR, once we decide we like the changes.TODO:
ImageHandleandLUTView. It seems to me likeImageHandleshould be aLUTView, however there are a few methods brought in from theViewableABC that are strange here. There's a bit of naming skew between them though, which would be nice to resolve.if self.modelchecks, which I find annoying. They exist because theLUTViewdoes not require the model at construction time - this would require us pass aLUTModelto functions on e.g. anArrayView, which I found strange. But maybe that's less annoying?