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

Would you be willing to sign the ECA to make your contributions available for Eclipse? #107

Closed
keilw opened this issue Jun 13, 2017 · 13 comments
Assignees
Labels

Comments

@keilw
Copy link
Member

keilw commented Jun 13, 2017

Hi,

Thanks again for your contributions to uom-systems, especially the UCUM part.
I made relevant system modules available as P2 repository on https://bintray.com/unitsofmeasurement/p2/systems.uom_p2_site

However, for Eclipse projects to use it, e.g. those under https://science.eclipse.org/ everyone who contributed something has to sign a so-called Eclipse Contributor Agreement (https://www.eclipse.org/legal/ECA.php), that would be mostly @magnonasc

Would that be OK for you? Otherwise only the "late binding" Eclipse UOMo implementation that's already under Eclipse License would be usable. UOMo 0.8 combines uom-se and UOMo, but right now it only uses https://github.com/unitsofmeasurement/uom-systems/tree/master/quantity (which @magnonasc also contributed, to, so without having to remove certain quantity types Eclipse also requires his ECA for that, there is just one per contributor not per module ;-)
TIA,
Werner

@keilw keilw added the question label Jun 13, 2017
@keilw keilw changed the title Would you be willing to sign the ECA to make systems-ucum available for Eclipse? Would you be willing to sign the ECA to make your contributions available for Eclipse? Jun 13, 2017
@garretwilson
Copy link
Member

@keilw you know that I'm a big promoter of your project, and I even poured a lot of money into it along with a developer to get UCUM working. But at the end of the day the uom-systems architecture doesn't correctly handle UCUM. The unit tests still break. Whatever you say in reply, the unit tests still break. We discussed this in extensively in #97.

So at the moment I'm leery of associating my name and my company's name with a project that is broken. The project does not work for UCUM. It does not work.

So maybe you could provide an update on #97 and let me know when you will fix the uom-systems architecture so that it supports UCUM. You know that I am ready and willing to support this project, but I'm not ready to promote something that is broken, doesn't work, and you don't know when it will work.

@keilw
Copy link
Member Author

keilw commented Jun 13, 2017

Actually it's all working in master based on https://github.com/unitsofmeasurement/uom-se/releases/tag/1.0.7 released about a week ago.
Every single test against the UCUM system passes, even weird but correctly formatted things like
assertEquals("m[gil_br]", FORMAT_CS.format(MILLI(UCUM.GILL_BRITISH)));

uom-systems isn't just about UCUM and while I may not add all the UCUM units missing yet, there is just one issue open for https://github.com/unitsofmeasurement/uom-systems/milestone/4. Target date is the 17th, so the end of this week. I want to add all reasonable units from Unicode CLDR 31/ICU 59 (not every multiple and submultiple is reasonable, you can do all combinations with MetricPrefix) into this release. Given the Unicode CLDR system is used by Parfait and a couple of projects (also some customers e.g. in a large "Industry 4.0" environment) I want to get as much as useful into CLDR. Keeping further UCUM units for a 0.7.1 release, but those that are there and were tested pass 100% of those tests.

There are at least 3 or 4 contributors to systems-quantity. One type is not even used, so it could be moved into another module if the worst comes to the worst. Every quantity type we could not get approved would have to be substituted with Dimensionless or something written from scratch. However there is no great rush. Eclipse can be a bit slow especially when its own Release Train is rolling this month, so no extreme hurry. If systems 0.7.1 makes you feel confident enough, then happy to discuss it further after its release.

@magnonasc
Copy link

Heey @keilw, thank you for remember of me, and @garretwilson, thank you for assume it while I was busy with other projects.

I took some time to test the units, and although they seem well formatted at first, it's not completely working yet. It will probably be an easier work than the one that we've been through (and I couldn't help to fix, for known reasons). Here is what I tested, I used a simply System.out.println to make it faster, but it would be better to add it as JUnit statements:

System.out.println(FORMAT_CS.format(MICRO(LITER))); //it works it's already in the log
System.out.println(FORMAT_CS.format(MICRO(LITER_DM3))); //it works OUT: µl
System.out.println(FORMAT_CS.format(GILL_BRITISH)); //it works it's already in the log
System.out.println(FORMAT_CS.format(MICRO(GILL_BRITISH))); //it works OUT: u[gil_br]

And the prefix is being formatted pretty good in this case:

System.out.println(FORMAT_CS.format(LITER.multiply(10))); //it works, it formats the prefix perfectly! OUT: µL

But if I combine some multiplications things get weird:

System.out.println(FORMAT_CS.format(MICRO(LITER).multiply(10))); //it doesn't work OUT: dauL

If I simply multiply something that's in the range of a prefix, it'll format again, and won't combine with the current prefix that's already there.
If I keep multiplying for something within the range it keeps adding it again, and again, and again...
For example:

System.out.println(FORMAT_CS.format(MICRO(LITER).multiply(10).multiply(10).multiply(10).multiply(10).multiply(10))); //OUT: dadadadadauL

So although it's functional with simple cases, where the user would simply get the unit with the prefix already, it's not functional for operations with it.

You need to fix it before saying that it's everything working, but it looks pretty good @keilw, you (and the people who contributed with the prefixes issue) made a great progress there, it's great to see that!

@keilw
Copy link
Member Author

keilw commented Jun 17, 2017

@magnonasc Thanks for trying it and the detailed analysis.
The problem of chained operations does not seem specific to the UCUM system. Please raise a ticket in https://github.com/unitsofmeasurement/uom-se/issues
I added a test in https://github.com/unitsofmeasurement/uom-se/blob/master/src/test/java/tec/uom/se/unit/PrefixTest.java

assertEquals(MICRO(GRAM), GRAM.divide(1000000));
//      assertEquals(MICRO(GRAM), GRAM.divide(1000).divide(1000));

The first one passes, the second currently won't. We'll have to see, how chained operations can be supported in a future version. And what level of nesting is reasonable.
Of course one could break above statement into something like
GRAM.divide(10).divide(10).divide(10).divide(10).divide(10).divide(10));
Whether we'll support this endlessly in an open source implementation, I can't say for sure. There are always areas that commercial closed-source implementations or libraries are more than happy to compete.

Also please advise what you'd expect, dauL is not wrong, but where chaining prefixes does not make sense, it would have to be something like uL*10 for MICRO(LITER).multiply(10).
Maybe you hope to see something like MILLI(LITER).divide(1000) end up being uL again? Not sure, if it'll work for all cases, again this could mean asking the same for MILLI(LITER).divide(10).divide(10).divide(10). There are limits to draw.
In fact the UCUM spec under http://unitsofmeasure.org/ucum.html#section-Semantics §10 nested terms states

Parenthesized terms are not considered unit atoms and hence must not be preceded by a prefix.

So combining something like .multiply(10) with MICRO(LITER) whether in front or behind it seems illegitimate at least for UCUM. We probably won't prevent doing it by throwing an exception but it does not look like it should be supported in special ways either. As mentioned putting other operations behind it as far as possible sounds more than enough.

@keilw
Copy link
Member Author

keilw commented Jun 19, 2017

@magnonasc Please see the new ticket #108. If you have time feel free to comment or make suggestions there. This ticket is only to track the ECA issue which needs to be resolved sooner or later.

@keilw
Copy link
Member Author

keilw commented Feb 20, 2018

@garretwilson @magnonasc Hi,
Just wanted to come back to this ticket as the reusable quantities including the likes of Information become extremely valuable for use by Eclipse projects like UOMo (which already does) or MicroProfile Metrics. The formatting of UCUM was solved as good as it gets right now. Maybe based on JSR 385 and the new universal RI Indriya we can pick it up again, but right now there are other tasks and issues like the SI reforms next year. If @magnonasc is not able to sign the ECA, I'm afraid I have to delete Acidity or move it to some "sandbox" module instead of the official quantity collection. Eclipse IP will demand an ECA from everyone who touched the code at least in the quantity modules. UCUM should be fine for now.

@garretwilson
Copy link
Member

Thanks for the reminder, @keilw . @magnonasc and I will address this this week.

@garretwilson
Copy link
Member

Hi, @keilw . I was worried about the personal information on the agreement, but checked with the Eclipse Foundation and they said they keep it confidential.

But the thing that confuses me is that I don't see anything on the agreement that specifies which Contribution is being referred to. It says:

"Contribution" shall mean any original work of authorship, including any modifications or additions to an existing work, that is submitted by You to the Eclipse Foundation for inclusion in, or documentation of, any of the Eclipse Foundation projects

So I guess that just means "anything you submit to Eclipse, and Werner is doing that for you"?

@keilw
Copy link
Member Author

keilw commented Feb 26, 2018

Yes, Eclipse Foundation does not bind this to a project at least not for he ECA, otherwise it could be like JCP where the old "Exhibit B" had to be approved for every JSR.

@keilw
Copy link
Member Author

keilw commented Mar 7, 2018

How's the progress with the ECA? I am just about to upload the sources of system-quantity-0.7.2 which contains @magnonasc's contribution. Depending on how thorough the IP team is on it, every contributor who is mentioned in a file could be checked for their ECA. If the worst comes to the worst we would have to patch the library and remove unsanctioned quantities, but I hope we don't have to. TIA

@unitsofmeasurement unitsofmeasurement deleted a comment Mar 7, 2018
@magnonasc
Copy link

@keilw, I signed the ECA. Sorry if you received a notification from another account.

@keilw
Copy link
Member Author

keilw commented Mar 7, 2018

@magnonasc Thanks, I have not, but I don't think Eclipse does that for every project. The libraries were both approved with their CQ, so that also confirms they have everything they need. Thanks a lot for signing it. As contributor if you were interested, that also would give you an opportunity to contribute to Eclipse UOMo including the "late binding" but very compatible UCUM libraries that AFAIR pass all the UCUM TCKs as well.

@keilw
Copy link
Member Author

keilw commented Mar 7, 2018

I close this ticket now as it was to track the ECA.

@keilw keilw closed this as completed Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants