Make stretches be customizable on a layer by layer basis #2453
Conversation
…tretch_parameters callback properties on a state class and define stretch_object, keeping everything in sync
| for key, value in self.stretch_parameters.items(): | ||
| if hasattr(self._stretch_object, key): | ||
| setattr(self._stretch_object, key, value) | ||
| else: | ||
| raise ValueError( | ||
| f"Stretch object {self._stretch_object.__class__.__name__} has no attribute {key}" | ||
| ) |
There was a problem hiding this comment.
If a stretch accepts multiple attributes but needs to validate them wrt each other (for example: x/y coordinates of knot locations for a spline), what would be the best way to handle that?
As this stands, those will need to be passed as a single entry (stretch_parameters['knots'] = ([x1, x2], [y1, y2])) so that validation can occur in a single setter on the stretch object.
Alternative ideas:
- support some sort of context manager (similar to
hold_sync) that validates the attributes after setting all of them at the end of this for-loop? This of course needs to be compatible with the astropy stretch objects that are used by default as we probably can't modify those upstream just to account for this. - instead of looping and checking for
hasattron the individual keys, could we instead require aupdate_parametersmethod (ifstretch_parametersis not empty) on the stretch object which handles this logic internally by getting the entire dictionary in one pass?
There was a problem hiding this comment.
Just to put another idea out there, could you perhaps not validate when the attributes are set but when the stretch is called?
There was a problem hiding this comment.
That's an option, but probably causes unnecessary cost when calling the stretch repeatedly. In our case, we can probably just validate externally since we manage the stretch object, but then if a user modifies the values, they will need to make sure they make sense.
|
Is this patch not backward compatible? |
|
I did try and make it backward compatible but may have missed something. I will investigate the jdaviz failures tomorrow. |
|
I think it is ok. I ended up doing a version check and it seems to work. See spacetelescope/jdaviz#2591 |
For the image and scatter viewers, this adds a new dictionary property
stretch_parameterswhich can be used to set stretch parameters specific to the layer. These parameters are then set as attribute on state instances. So the requirement for new stretch classes is that parameters can be set as attributes on existing instances (e.g.LogStretch.exp). Example:Glue deals with keeping
stretch_parametersin sync with the stretch instance/object, and will deal with refreshing the plot.