Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting stretch as a string is problematic if the stretch requires a parameter #146

Open
mwcraig opened this issue Jul 17, 2021 · 5 comments
Labels
API Issues that relate to the API itself rather than implementations question Further information is requested

Comments

@mwcraig
Copy link
Member

mwcraig commented Jul 17, 2021

While setting the stretch using a string (e.g. viewer.stretch = 'sqrt') is very convenient, saome stretches require parameters (e.g. PowerStretch) and many have optional parameters (e.g. AsinhStretch).

I don't see a way to handle that without one of these:

  1. add some more public properties that correspond to the settings for these stretches
  2. make a method set_stretch instead that allows optional parameters, and a get_stretch
  3. Allow stretch to be set to either a string or an astropy stretch object.

Of these, 1 seems confusing, 2 is straightforward, 3 is maybe more convenient in many cases.

@mwcraig mwcraig added question Further information is requested API Issues that relate to the API itself rather than implementations labels Jul 17, 2021
@mwcraig
Copy link
Member Author

mwcraig commented Jul 17, 2021

Actually, the original implementation called for an Astropy stretch to be the value, I think it was turned into a string to get ginga working quickly, maybe?

@mwcraig
Copy link
Member Author

mwcraig commented Jul 17, 2021

After looking this over some more I think it makes sense to go with 3....or maybe even disallow strings.

@mwcraig
Copy link
Member Author

mwcraig commented Jul 17, 2021

I see in #126 that there is no restriction imposed on what stretch can be. It might be nice to be explicit about what is allowed, though.

@pllim
Copy link
Member

pllim commented Jul 19, 2021

2 and 3 are basically the same solution, but just a matter of preferring method over attribute setter, or vice versa.

👎 on disallowing string for Ginga though. I don't see the point to have to manually build an astropy.visualization object when I can just pass in a string to it.

@mwcraig
Copy link
Member Author

mwcraig commented Jul 19, 2021

👎 on disallowing string for Ginga though. I don't see the point to have to manually build an astropy.visualization object when I can just pass in a string to it.

Agree, even for things other than Ginga. Almost all of the astropy stretches have defaults for optional parameters, so something like https://github.com/astropy/astrowidgets/pull/142/files#diff-6aea2ce078dbeb19a51bdb4018c2b963ffa8728ce163c5d91aeb3bf42a3a2293R337 works as long as users can change properties after the fact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues that relate to the API itself rather than implementations question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants