-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for Booleans #8
Conversation
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 presume most of this is just cut-paste-adapted from the other types, so it is likely to work the same. However, there is a question of unit tests: there aren't any. Maybe we should write some quick down-the-middle tests for all the existing types including boolean, just as an extra layer of caution?
src/main/java/net/imglib2/img/basictypelongaccess/wrapped/WrappedBooleanLongAccess.java
Outdated
Show resolved
Hide resolved
public long getAddres() | ||
{ | ||
return wrapped.getAddres(); | ||
} |
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.
Typo, might need to be fixed in wrapped
as well. Is this a typo that already exists throughout imglib2-unsafe
?
public long getAddres() | |
{ | |
return wrapped.getAddres(); | |
} | |
public long getAddress() | |
{ | |
return wrapped.getAddress(); | |
} |
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.
Yeah, I saw that too @hanslovsky, but it's all over the place. Here's another example.
I figured this would be a breaking change to address, so I didn't fix it.
I suppose we could write getAddress
and deprecate getAddres
😅
@ctrueden what would you want to test? All of this functionality assumes you have an address in memory to operate on. How do we get such an address. That said, I'd love to test this functionality. I just don't know how. |
Here's another point of discussion: in the short term, we don't actually have a use case for the vast majority of this code. I actually only want the But we may be able to use the |
@gselzer To allocate a block of memory, I believe you can use the Alternately, there is probably a way to use |
@ctrueden please let me know what you think of these tests |
b6c2444
to
67ba039
Compare
So @ctrueden I'm actually going to remove most of the code out of this PR. We have no use for any of this I'd want to support some type of
For these reasons, I want to basically only keep the |
This is actually a much more useful way of doing things, particularly when wrapping NumPy data
Since they are the same size as a ByteUnsafe, let's just wrap that
One thing I'd like to draw attention to is the naming of I named these new classes as such since there is no definite connection between But, this leads to a bit of confusion. For example, we can make an So: should we name these consistently, or would that make too strong of a connection between these objects? |
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.
@gselzer Thank you very much for your work on this PR! It is mostly fabulous. 🎇 My only concern is the naming of the Unsafe
-backed boolean access, which I argue should be named BooleanUnsafe
, not NativeBooleanUnsafe
.
@gselzer If you agree, you can either push a commit renaming it, or else just give me a thumbs up and I will do it myself, before merging.
src/main/java/net/imglib2/img/basictypelongaccess/unsafe/NativeBooleanUnsafe.java
Show resolved
Hide resolved
This class is the Unsafe analogue to ImgLib2 core's BooleanAccess class in the net.imglib2.img.basictypeaccess package. So it should be named BooleanUnsafe, same as e.g. int with IntAccess in core and IntLongAccess and IntUnsafe here in imglib2-unsafe.
Like some others, it is not used, but for consistency it should exist.
This PR adds support for
Booleans
.This PR is motivated by the desire to support the conversion to/from
RAI<BooleanType>
in PyImageJ.