search

imagej / imagej-common

ImageJ core data model
https://imagej.net/libs/imagej-common
BSD 2-Clause "Simplified" License
9 stars 18 forks source link

Allow conversion of Numbers into RealTypes #103

Closed gselzer closed 1 year ago

gselzer commented 2 years ago

This PR adds support for conversions from Numbers into RealTypes, for example:

ij.convert().convert(new Byte(1), RealType.class)

This PR does not support the following conversion:

ij.convert().convert(new Byte(1), ByteType.class)

That conversion is already handled by DefaultConverter.

The motivation behind this PR lies with Ops: Ops that take a RealType cannot currently be called with some Number implementation. This PR should fix that.

There is the question of whether we should be explicitly performing that conversion (i.e. writing dedicated Converters for it). I don't feel strongly: it feels more correct, but it is also a lot of boilerplate code (especially if we want to support all RealType implementations) and we'd have to get crafty with the Priority of those Converters.

gselzer commented 1 year ago

You know, I should actually add support for BooleanType and IntegerType as well...

ctrueden commented 1 year ago

I pushed a commit cleaning up imagej-common's converter type hierarchy. It introduces a ConciseConverter base class that makes it simpler to write converter plugins. It also fixes a bug in the ROITreeToMaskPredicateConverter that was checking destination assignability in the wrong way, and I added comments explaining in detail why code is the way it is.

I still have to go through some of my other comments above. I also may want to change some details of how ConciseConverter is laid out. Will keep working on it, but feedback on what I did so far (1f0e3c28578192778640789a9c983b0d09aa1875) is welcome.

ctrueden commented 1 year ago

There are two things left to resolve before this branch can be merged:

  1. Check whether UnsignedLongTypeToBigIntegerConverter is obsoleted by the more general IntegerTypeToBigIntegerConverter.
  2. Decide whether to assert something about the return values of convertService.convert in NumberAndTypeConversionTest (there are three places where the return value is not currently captured).
ctrueden commented 1 year ago

All above issues addressed! LGTM

imagesc-bot commented 1 year ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/converting-arrayimg-to-imageplus-for-using-the-analyze-skeleton-plugin/85934/3