NAPI SDRClassifier Integration

numenta / htm.java

Hierarchical Temporal Memory implementation in Java - an official Community-Driven Java port of the Numenta Platform for Intelligent Computing (NuPIC).

GNU Affero General Public License v3.0

308 stars 160 forks source link

NAPI SDRClassifier Integration #511

Closed Hopding closed 7 years ago

Hopding commented 7 years ago

Fixes #512.

[x] Add a Classifier interface.
[x] Add a new KEY that maps inferred field names to the desired Classifier type for that field: INFERRED_FIELDS(“inferredFields”, Map<String, Classifier.class>)
[x] Need to add code so that when the KEY.AUTO_CLASSIFY key is found to be true, a lookup is performed on the Parameters object for that node to retrieve the fields flagged to be classified.
[x] Errors should be thrown if KEY.AUTO_CLASSIFY is specified and there are no values for the KEY.INFERRED_FIELDS or it is null.
[x] Add tests for the presence and throwing of appropriate exceptions, and add tests for ability to specify classifiers.

cogmission commented 7 years ago

Hi Andrew, Nice Job!

Remember you'll need to make an "Issue" to explain the work and then reference it here (in the Pull Request), as "Fixes ###" so that the above "Fixes Validator" will pick it up.

Also, I'm sure you know this but just in case, you'll need tests for this work to show everything is in order?

Congrats! and Thank you! -D

cogmission commented 7 years ago

@Hopding Looks like your IDE threw in some "wrong" imports for Param.class and InvalidStateException.class -> you'll need to erase those manually and import the right ones?

Otherwise I'll do a review tomorrow?

Hopding commented 7 years ago

@cogmission Yeah, I'll take care of that. I've also got to update some of the tests since they are no longer passing (because they use KEY.AUTO_CLASSIFY but dont specify anything for KEY.INFERRED_FIELDS).

cogmission commented 7 years ago

...also I don't know if you thought of this already, but thinking about it - I'm getting pretty excited, and was thinking it would be nice to have a test that shows having more than one classified field where each is using a different classifier? (kind of obvious test, sorry) - but the functionality you're doing is very cool and did I say I was excited about it? 😄

rhyolight commented 7 years ago

Great contribution, @Hopding!

Hopding commented 7 years ago

@cogmission I'm also excited to get this functionality working so that we can use the SDRClassifier with the NAPI. I think I've covered that test you mentioned here (let me know if you're looking for something more/different?).

I've just pushed a commit updating the tests to get them passing again. So, all else withstanding, this PR should be ready for review.

Hopding commented 7 years ago

Looks like I forgot some tests. Will get those updated too...

coveralls commented 7 years ago

Coverage increased (+0.04%) to 83.096% when pulling 650ba0e6a511ff498f55c3208c77b8c4d412c35d on Hopding:napi-sdrclassifier-integration into 90777955ab5c5c0c5ff29394e02ceeeeadc63cc0 on numenta:master.

cogmission commented 7 years ago

@Hopding Nice. Ok I'll have a look when I get home tonight...

cogmission commented 7 years ago

@Hopding Nice Job!

I left some comments in the "Files Changed" view... In general...

1.) Let's keep to the standard of using explicit import statements. 2.) Please remove the requirement to specify KEY.AUTO_CLASSIFY as "false". False should be accepted but it should also interpret the missing key as false in addition to avoid the verbosity of having to set it every time?

coveralls commented 7 years ago

Coverage increased (+0.04%) to 83.096% when pulling 4cb2fafabb5878441fd96da9da4b1c486dd05c67 on Hopding:napi-sdrclassifier-integration into 90777955ab5c5c0c5ff29394e02ceeeeadc63cc0 on numenta:master.

coveralls commented 7 years ago

Coverage increased (+0.07%) to 83.124% when pulling 4cb2fafabb5878441fd96da9da4b1c486dd05c67 on Hopding:napi-sdrclassifier-integration into 90777955ab5c5c0c5ff29394e02ceeeeadc63cc0 on numenta:master.

coveralls commented 7 years ago

Coverage increased (+0.04%) to 83.096% when pulling f44cc5a0566bc9365090b5a22b217a88579cfcf1 on Hopding:napi-sdrclassifier-integration into 90777955ab5c5c0c5ff29394e02ceeeeadc63cc0 on numenta:master.

coveralls commented 7 years ago

Coverage increased (+0.04%) to 83.096% when pulling a3c62b76e8d2b87da90a69f9ebd3eeb643669cee on Hopding:napi-sdrclassifier-integration into 90777955ab5c5c0c5ff29394e02ceeeeadc63cc0 on numenta:master.