prmr / Creco

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

Design of the AttributeSelector #72

Closed prmr closed 10 years ago

prmr commented 10 years ago
asutcl commented 10 years ago

After talking with Edward and Gabriel, I will make a @Component Bean out of the ScoredAttribute. AttributeExtractor will become a @Controller Bean, it will have no more fields. The interface will be roughly the same. I will take care of adapting the calls to Attribute Extractor once the change is made.

prmr commented 10 years ago

Why will AttributeExtractor become a @Controller (as opposed to just a @Component)?

asutcl commented 10 years ago

It could be a @Component and hold all the index ScoredAttribute lists for all Categories. Althought, my idea was to create a new class for the Bean that holds the map from Category.id to Collection<ScoredAttribute> and then have the AttributeExtractor be a controller that fetches the Collection<ScoredAttribute> from a Category.id in this Bean and then sorts it according to a default or passed sorting field (score).

But AttributeExtractor could always be @Component and directly return the correct list, if we think that is preferable.

Either way ScoredAttribute will become immutable since it will become a singleton.

MariamN commented 10 years ago

@asutcl is there a way to check the type of a scored attribute directly without getting the DefaultValue first?

asutcl commented 10 years ago

@MariamN now there is. You can check with isBoolean, isNumeric, isString, isNA. You can also always get the default value which will be the mean or the mode, but you can't set a new default value anymore. I wouldn't recommend checking type against the default value though. Please use the new an improved is methods.

This is in branch Issue72, for the moment. Should be merged soon.

Thanks for pointing this out. I thought I had written these already.

asutcl commented 10 years ago

I merged master into Issue72. I fixed all merge issues. I have an error on TestFiltering due to a Bean injection problem. I am not sure what to do with it right now. The application runs no problem. Can anyone take a look at it maybe @forgues, I know you have more experience with these. (branch Issue72)

Should I push to master, even if 1 test fails?

forgues commented 10 years ago

I'm having issues committing my changes to git... You need to add two beans to the test file src/test/resources/META-INF/test-context.xml.

<bean id="TestServiceFacade" class="ca.mcgill.cs.creco.logic.ConcreteServiceFacade" />

<bean id="TestAttributeExtractor" class="ca.mcgill.cs.creco.logic.AttributeExtractor">
        <constructor-arg index="0">
            <ref bean="TestDataStore"/>
        </constructor-arg>
</bean>

If you just add those two beans in the file and run the tests, everything should pass. At least, everything passed when I added it into your branch, but I can't commit right now for some reason.

asutcl commented 10 years ago

I changed it and it worked thanks!

Is it only an issue with my branch or with all other branches. If it is a restriction on my branch only I will try to fix it before merging into master?

forgues commented 10 years ago

Don't worry, it has nothing to do with your branch. You can merge with master when you're ready.

On my desktop I can't checkout branches as a local copy anymore. When I do Team -> Switch to -> checkout branch, I don't even get a dialog prompt asking if I want to checkout a local copy. Instead, it directly checks out the commit. Anyways, I don't have the issue on my laptop, and I should be able to figure it out eventually.

asutcl commented 10 years ago

Ok good.

I've merged with master.