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

Represent None component.units as empty strings #2356

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

dhomeier
Copy link
Collaborator

@dhomeier dhomeier commented Jan 31, 2023

Description

To fix issues as in spacetelescope/jdaviz#1909 where f'{unit}' returns unexpected results, this returns self._units of None as '' for all Component subclasses. This is analogous to how these are already handled in Coordinates.
In case the internal value of self._units == None is still required somewhere, the second commit reverses the transformation on setting units, but it does not seem to be required for passing tests.

@pllim
Copy link
Contributor

pllim commented Feb 1, 2023

What was the design decision to use None in the first place? astropy.units would happily parse '' into u.dimensionless_unscaled.

@astrofrog
Copy link
Member

I think I'm happy for the public API to not change, I just wanted to be be able to distinguish internally between units being set vs default.

@pllim
Copy link
Contributor

pllim commented Feb 1, 2023

Is this related?

DeprecationWarning: Passing np.nan to mean no clipping in np.clip has always been unreliable, and is now deprecated. In future, this will always return nan, like it already does when min or max are arrays that contain nan. To skip a bound, pass either None or an np.inf of an appropriate sign.

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 1, 2023

I think I'm happy for the public API to not change, I just wanted to be be able to distinguish internally between units being set vs default.

Then perhaps it's better to cherry-pick the first commit, so passing units='' would not be translated to None.

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 1, 2023

Is this related?

DeprecationWarning: Passing np.nan to mean no clipping in np.clip has always been unreliable, and is now deprecated. I

To the units treatment? No, showed already up before the units PR was merged; I suspect it is related to the

glue/glue/viewers/scatter/layer_artist.py:60: RuntimeWarning: All-NaN slice encountered
return 10. ** (np.log10(np.nanmax(array)) * self.contrast)

from the same test_density_map_incompatible_subset, but can't tell where exactly this is raised.

@astrofrog
Copy link
Member

Then perhaps it's better to cherry-pick the first commit, so passing units='' would not be translated to None.

@dhomeier - sounds good, I'll remove the second commit

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other jdaviz failures are unrelated to this PR, so merging this and will open a separate PR to fix the other issue.

@astrofrog astrofrog merged commit 4d28eb0 into glue-viz:main Feb 2, 2023
@dhomeier dhomeier deleted the return-units-empty branch February 11, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants