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

Improve the units API #1684

Merged
merged 3 commits into from
Mar 25, 2015
Merged

Improve the units API #1684

merged 3 commits into from
Mar 25, 2015

Conversation

ctrueden
Copy link
Member

Specifically, this branch makes the Quantity superclass nicer to work with, and adds some helpful constructors with default unit specifications, to avoid errors when converting downstream code to the new API.

See here for an example where these changes would be helpful.

/cc @qidane
--no-rebase

This is very convenient when coding API against Quantity variables
without knowing their type; otherwise, one must needlessly special case.
@ctrueden
Copy link
Member Author

I don't love the Unit<? extends Quantity> unit() declaration, but it's the best you can do without introducing a generic parameter to Quantity.

@ctrueden
Copy link
Member Author

As an aside, it is a shame that the OME codebase could not adopt a standard units library. We will need to create an integration layer to bridge OME units to and from SciJava units. See scifio/scifio-ome-xml#23

@ctrueden
Copy link
Member Author

OK, I see that there are a bunch of new FormatTools method signatures that return new quantities with default units. And I guess, if the default unit differs in the schema depending on which field it is, then the new constructors I added above make less sense and are in fact dangerous. Interested to hear others' thoughts.

@melissalinkert
Copy link
Member

I'd prefer to see 17300c4 reverted, but the other changes read fine.

@ctrueden
Copy link
Member Author

@melissalinkert Are there ever two fields in OME-XML that use the same quantity but different units? If so, I will purge the commit. But if not, what's the harm?

If we do eliminate that commit, I would still like to see shorter API calls to create new quantities. As things stand, it is a little verbose to write meta.setPhysicalSizeX(FormatTools.getPhysicalSizeX(pSizeX).

@melissalinkert
Copy link
Member

PhysicalSizeX on Pixels, PositionX on Plane, and EmissionWavelength on Channel all use the Length type, but with different default units (microns, reference frame, and nanometers, respectively).

@ctrueden
Copy link
Member Author

Thanks @melissalinkert, I purged 17300c4 from the branch.

@melissalinkert
Copy link
Member

Thanks. All reads OK now, just waiting on inclusion in the merge builds.

melissalinkert added a commit that referenced this pull request Mar 25, 2015
@melissalinkert melissalinkert merged commit f3c7bc3 into ome:develop Mar 25, 2015
@sbesson sbesson added this to the 5.1.0 milestone Apr 2, 2015
@ctrueden ctrueden deleted the devel/units-api branch December 29, 2016 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants