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

with channels, math operators should consider null, signed properties #1079

Open
ddkohler opened this issue Jun 17, 2022 · 5 comments
Open

Comments

@ddkohler
Copy link
Contributor

e.g. for log, we should set a reasonable null value (because the old null will almost certainly be unreasonable), and we warn or prevent the operation if data is signed:

def log(self, base=np.e, floor=None):
    """calculate log of channel element-wise

    Parameters
    ----------
    base : number (optional)
        Base of log. Default is e.
    floor : number (optional)
        Clip values below floor _after log_. Default is None.
    """
    if self.signed:
        raise warnings.Warn(f"Channel {self.name} is signed and log is invalid for negative numbers. \
            Consider using symlog. ")
    super().log(base=base, floor=floor)
    self.null = floor if floor is not None else self.min()

Warnings could be issued for */+- if nulls differ.

Perhaps symmetric_root should also perform the sqrt operation relative to null?

@ddkohler
Copy link
Contributor Author

ddkohler commented Jul 6, 2022

A central problem with my proposed solution here is that null not equivalent to floor; we are just using the null property to make plotting work, which is rather roundabout. By just rewriting null we ignore lots of subtlety in how we gauge the number scale of the channel. It's just problematic to do math operations and then guess what the new "scale" is that we wish to use.

Here's an alternative proposal: we replace all plotting scale "hints" (null, extent, signed) with actual norm objects. These norm objects should be similar to (or identical to) matplotlib norm objects. The norm objects have different properties depending on the type of norm

If we like this route, the implementation should naturally resolve Issue #1062 .

@untzag
Copy link
Member

untzag commented Jul 6, 2022

Love the idea @ddkohler

@ksunden
Copy link
Member

ksunden commented Jul 6, 2022

Note that warnings do not use the raise keyword... they are a weird thing that like kind of is an exception, but if you want them to only warn you don't use raise

Not quite sure what symmetric_root relative to null actually is, mathematically (would it be sign*sqrt(abs(x-null)), would you add null back at the end?

as for norm objects, I kind of like this idea, though not sure how easily those would be encapsulated in e.g. attrs or what have you like we do with null and signed now (and is nice for the "its all in the file" approach we have)

How would norms affect access to the data? would it only affect plotting? for instance?

I have a lot of questions for things that I don't fully know what the answer is...

@ddkohler
Copy link
Contributor Author

ddkohler commented Jul 6, 2022

Not quite sure what symmetric_root relative to null actually is, mathematically (would it be sign*sqrt(abs(x-null)), would you add null back at the end?

Yeah, that would be my best implementation guess. But I think you have hit on the ambiguity that troubles this issue. When we give a channel a "null" value, is it merely a hint for plotting, or do we really mean to say that null is to be taken as the zero of the channel's number scale? My current leaning is the former interpretation, so math functions should not adapt to null values or similar properties. For math, null is always zero.

as for norm objects, I kind of like this idea, though not sure how easily those would be encapsulated in e.g. attrs or what have you like we do with null and signed now (and is nice for the "its all in the file" approach we have)

That will be the trick. I think all matplotlib norm objects are rather simple, so putting norm_type and norm_properties in the attrs would be a acceptable approach?

How would norms affect access to the data? would it only affect plotting? for instance?

I think norms are only there for plotting. I might be wrong, but that is basically all null, signed, extent (others?) are used for currently, correct?

I have a lot of questions for things that I don't fully know what the answer is...

Yeah, this would be a significant change. If we go this route, I think we need to keep current channel properties and develop a phase out plan for future versions.

@ksunden
Copy link
Member

ksunden commented Jul 6, 2022

That will be the trick. I think all matplotlib norm objects are rather simple, so putting norm_type and norm_properties in the attrs would be a acceptable approach?

The trouble is that HDF5 attrs do not allow dictionary types, so it may need to be serialized to a string or something... its one of few the things I dislike about the HDF spec

I think norms are only there for plotting. I might be wrong, but that is basically all null, signed, extent (others?) are used for currently, correct?

Mostly, major_extent and minor_extent (which use null in their computation) may be independently useful (but these in turn are also mostly used, especially internally, for plotting), and for normalize which maps 0-major extent to 0-1 (possibly signed, I think) and edits the data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants