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

Streamlining axis size #545

Conversation

Tomaz-Vieira
Copy link
Contributor

@Tomaz-Vieira Tomaz-Vieira commented Dec 8, 2023

This PR tries to streamline axis sizes, so that there are fewer special cases and so that it is easier to verify their correctness:

  • All axes sizes are FixedSize | ParameterizedSize | SizeReference;
    • No more ints or strs or None or whatver;
  • Checking that the axes sizes are good is done in one go, after resolving all references;
  • Unit is only specified in the non-reference sizes, so they can't conflict with the referent;
  • Channel names are streamlined; either give all names or a reference and prefix/suffix;
  • Reference-to-reference-to-reference... is supported;
  • There is reference-loop detection;
  • Less code;

TODO:

  • Check that channel axes don't reference non-channel axes
  • Special-case for batch axes? Does it come into play when looking at the test tensors?
  • Probably move TensorDataDescr into ChannelAxis

@FynnBe
Copy link
Member

FynnBe commented Dec 13, 2023

Special-case for batch axes? Does it come into play when looking at the test tensors?

For simplicity and consistency we (should) expect the test tensors to have singleton batch axes if their description is it. In other words, the test tensor should match the description exactly

Probably move TensorDataDescr into ChannelAxis

I don't think so. I wanted that initially, but then I realized that tensors may not have an explicit (singleton) channel axis. And we'd still need the data description

@@ -177,13 +176,35 @@ def validate_tensor_axis_id(s: str):

SAME_AS_TYPE = "<same as type>"

class FixedSize(Node):
extent: int
unit: "TimeUnit | SpaceUnit" #FIXME: generic?
Copy link
Member

Choose a reason for hiding this comment

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

moving the unit into the size means we break compatibility with https://ngff.openmicroscopy.org/latest/#axes-md
I think we should try to stay directly compatible.

Copy link
Member

Choose a reason for hiding this comment

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

this is also mixing the pixel size with the unit.
the unit should go with the scale. together they describe the distance of two pixels/frames.
The size is intended to describe the number of pixels/frames, i.e. the tensor shape

@@ -276,51 +311,48 @@ class WithHalo(Node):
class BatchAxis(AxisBase):
type: Literal["batch"] = "batch"
id: Annotated[AxisId, Predicate(lambda x: x == AxisId("batch"))] = AxisId("batch")
size: Optional[Literal[1]] = None
batch_size: Optional[Literal[1]] = None
Copy link
Member

Choose a reason for hiding this comment

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

why change the field name to batch_size?

Copy link
Member

Choose a reason for hiding this comment

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

ah.. found the size property..
how about we just use ParameterizedSize(min=1, step=1) as a default value and leave the field as size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about we just use ParameterizedSize(min=1, step=1) as a default value and leave the field as size?

But this would type this field as Optional[ParameterizedSize], which would allow it to be something like ParameterizedSize(min=3, step=10), and that seems to go against the original intention of having it be either exactly 1 or any number picked by the runtime

Copy link
Member

Choose a reason for hiding this comment

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

Ha, true!
How about

class ParameterizedBatchSize(ParametrerizedSize):
   min: Literal[1]
   step: Literal[1]

and

class BatchAxis....
    size: Union[Literal[1], ParameterizedBatchSize] = ...

?

@FynnBe FynnBe deleted the branch bioimage-io:pydantic_axes March 9, 2024 12:23
@FynnBe FynnBe closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants