-
Notifications
You must be signed in to change notification settings - Fork 1
Add Realtype builders #16
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
Conversation
petebankhead
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just made some quick comments for looking at the code (haven't tried running it).
I'm not certain this PR on its own makes things easier, since in the case where you can provide the type argument the you can already be sure where you have a RealType - and in the case where you can't, you're still getting an ImgBuilder<?, ?>.
I'm not certain, but I still have a suspicion that including the type T with ImgBuilder<T, ?> (rather than delaying its appearance to when an image is actually built) might be making it harder to get a friendly API. I think a caller would want an ImgBuilder<? extends RealType, ?> but I don't see any way for that to be specified... unless perhaps you subclass ImgBuilder.
| * @return a builder to create an instance of this class | ||
| * @throws IllegalArgumentException if the provided image has less than one channel | ||
| */ | ||
| public static ImgBuilder<?, ?> createRealBuilder(ImageServer<BufferedImage> server) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much this method helps the called when returning ImgBuilder<?, ?>, since they still seem stuck with the fact all they know for sure is ImgBuilder<T extends NativeType<T> & NumericType<T>, A extends SizableDataAccess>.
On the other hand, if they can provide the type to createBuilder then they would already know if they have a RealType or an ARGBType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature is now:
public static ImgBuilder<? extends RealType<?>, ?> createRealBuilder(ImageServer<BufferedImage> server)Is that better?
Similarly, the signature of the other unsafe function has become:
public static ImgBuilder<? extends NumericType<?>, ?> createBuilder(ImageServer<BufferedImage> server)
If you mean only return a
This isn't conclusive, because we might be better off moving away from any assumptions of a packed int representation (see e.g., qupath/qupath#1807) - but I'd be nervous about the API not providing us with any way to get an |
This is now what the unsafe |
|
I would assume with a high level of confidence that the packed int representation allows for a tonne of optimization that isn't possible otherwise, so I'd be inclined to keep it. The rest looks good to me insofar as I have a sufficient grasp of imglib2 and Java generics to be able to comment on it at all (not very far as it turns out). |
Add new functions
createRealBuilder()to create instances ofImgBuilderwith theRealTypetype. Basically, a RGB image server is converted to a 4-channelsUnsignedByteTypeimage (instead of being converted to a single-channelARGBTypeimage with the existingcreateBuilder()functions).This required to add a new
ByteBufferedImageAccessto getbytepixels from a RGBBufferedImage.The justification for this PR is that
RealTypeis quite easy to work with, unlikeARGBType.If this PR makes sense, shouldn't we delete the existing
createBuilder()functions? Would they still be used for anything?This PR is a draft until we agree on its content, because tests have to be added.