prmr / Creco

Recommendation System for Consumer Products
Apache License 2.0
6 stars 2 forks source link

Enhance TypedValue to parse intervals and units #57

Closed prmr closed 10 years ago

prmr commented 10 years ago

TypedValue should be able to parse strings that are numeric in essence but represent:

prmr commented 10 years ago

Just pushed the new interface specification to branch Issue0050. You can take over and implement the parsing in the constructor (and the tests), nothing else should need to change (except the Unit type).

priyasidhaye commented 10 years ago

Some ranges in Infant car seats have values like 32" or less. How do we convert this to a number?

priyasidhaye commented 10 years ago

Three type values have units : Numeric, String(range). @mangalagb

enewe101 commented 10 years ago

How is this going, did you work out what to do with 32" or less-type values?
When you're ready, you should try merging into Integration branch.

mangalagb commented 10 years ago

I'm not parsing values like the previous case. Since assigning an average to it would be incorrect.

mangalagb commented 10 years ago

I have modified the TypeValue class to include unit information. Now, every call to TypeValue must include the value whose type is to be inferred and the unit name associated with it. @asutcl and @MariamN make calls to TypeValue in your code. Can you please switch to branch typedValue and check if I'm passing the right values in your code?

prmr commented 10 years ago

The original intent was to have TypedValue automatically infer the unit name from the single-parameter constructor. Was this not possible?

prmr commented 10 years ago

In any case, why is the unit of type Object?

mangalagb commented 10 years ago

TypedValue only stores the value. Unit information is not present in it. It can be accessed via the unitName parameter. For example, STRING: 5-30 STRING: 30" or less STRING: 19-32" all have units. But they are not visible from TypedValue

prmr commented 10 years ago

Normally " is a shorthand for inches, hence my confusion.

mangalagb commented 10 years ago

But there are other units like Lbs which are not a part of the value field

mangalagb commented 10 years ago

Consider cases like NUMERIC: 17.0 in. NUMERIC: 19.0 lbs. The value just contains 17 or 19 . But the unit name is inches or lbs.

prmr commented 10 years ago

Yup, I see. In any case if that's the needed functionality it needs to go into today's release one way or the other.

mangalagb commented 10 years ago

If I change the constructor of TypedValue to include unit information, it will require changes to all code that make a call to Typedvalue including andrew and mariam's code.

prmr commented 10 years ago

Oh, I thought that was done already. This does not sound like a good idea minutes from the deadline. Anyways, please coordinate with them and enew101 directly.

mangalagb commented 10 years ago

Sure

mangalagb commented 10 years ago

There are other units in SmartPhone class like : NUMERIC: 0.2 GB NUMERIC: 5.8 oz.

asutcl commented 10 years ago

I don't have time to change my code tonight sorry. Determining the correct type I need before creating a TypedValue is non-trivial.

The reason I raised this issue in the first place was to try to make the most out of the data from the data base by being able to understand Strings such as 20 inches or 20" as numeric, and possibly also range Strings such as 10-20.

Presently the units aren't being used anywhere. Although it may be ideal, is necessary that every unit have a type. How can we attribute a unit to a field whose value is 10 in the data base?

Shouldn't AttributeExtractor still be able to create a TypedValue(10) without needing to specify a unit?

mangalagb commented 10 years ago

The case of 10-20 has been handled. It will return 15 as a single value now

enewe101 commented 10 years ago

@mangalagb would it be possible to have one of the unit types be unitless to avoid needing app-wide code modification?

In any case, would you be able to foresee when you will be ready to merge?

mangalagb commented 10 years ago

ok. As of now, I will avoid major changes to TypedValue to avoid modification to other parts of code handling the trivial cases only. I'l have it ready in 45 min max

prmr commented 10 years ago

This was built-into my original design. With aUnit == null, hasUnit() should return false.

mangalagb commented 10 years ago

As of now, TypedValue can handle ranges. ie. Strings like 20-30" or Limited to 13-15 Llb will be parsed and their average value returned. But support for units has not been added. Since this issue is only partially completed, I'm leaving it open.

enewe101 commented 10 years ago

Cool -- let's chalk units up for milestone 4! Alright, it looks like this is in Integration too. Thanks!

enewe101 commented 10 years ago

I made a special issue for unit comprehension, so I'm closing this one, ok?