Rostlab / JS16_ProjectA

In this project we will lay the foundations for our system by integrating data from multiple sources into a central database. The database will serve the apps and the visualization tool that will be developed in other projects.
GNU General Public License v3.0
28 stars 14 forks source link

Characters PLOD #129

Closed mina-kleid closed 8 years ago

mina-kleid commented 8 years ago

More documentation for the following character PLOD would be great

also currently all PLOD requests doesnt return anything

Legenzoo commented 8 years ago

@togiberlin Haven´t you implemented this?

sacdallago commented 8 years ago

There's actually no way to populate the PLOD collection. No NPM script, nor I see the collection on the db.

I NEED full hands on this issue, now.

@kordianbruck @Adiolis @togiberlin @boriside

sacdallago commented 8 years ago

You don't even have the dependency in the package.json?!?!!!!

What the fuck guys, who was responsible for this?????

sacdallago commented 8 years ago

@TheZoker @AlexMoroz @alschm

Legenzoo commented 8 years ago

Sorry, i can´t really help here, because i am not aware of the things that have been done according the PLOD. I am only as up-to-date as the issue #101.

I also see no possibility to fill the db by scraper or data json. So probably another group used the api @togiberlin implemented to fill the data?

I know that there has been some Plod data on @kordianbruck ´s server.

sacdallago commented 8 years ago

@Adiolis that might have been mock data, because one quick look at the repos of B illuminates us all:

  1. https://github.com/Rostlab/JS16_ProjectB_Group6/blob/develop/main.js that can be imported and called with the character name and the PLOD stored in the relevant collection
  2. https://github.com/Rostlab/JS16_ProjectB_Group7/blob/develop/npm.js same thing.

Literally: Import package, for every character get the prediction via the relevant function in the package, store it in the DB.

sacdallago commented 8 years ago

Actually, let me be even more specific:

  1. https://github.com/Rostlab/JS16_ProjectB_Group6/blob/develop/main.js#L16
  2. https://github.com/Rostlab/JS16_ProjectB_Group7/blob/develop/npm.js#L59

Don't get me wrong, I know you were not responsible for this. I know who was responsible for this. @TheZoker @togiberlin ( in part @CavidSalahov @AlexMoroz )

EDIT I know my chickens

Legenzoo commented 8 years ago

I notified the group on facebook. I can´t do that before Thursday. =/

TheZoker commented 8 years ago

@sacdallago Which dependency is missing in the package.json? I don't fully understand what's going on right now Do you mean the hole package of project B? https://www.npmjs.com/package/gotplod

sacdallago commented 8 years ago

@TheZoker https://github.com/Rostlab/JS16_ProjectA/blob/master/package.json#L20 There should be the two packages from B somewhere here, and someone in A should be using them to generate the PLODs

TheZoker commented 8 years ago

These are the missing dependencies, right?

sacdallago commented 8 years ago

Step one.

TheZoker commented 8 years ago

Am I responsible for anything else here?

AlexMoroz commented 8 years ago

@TheZoker use "latest" for package versions

sacdallago commented 8 years ago

@TheZoker no. thanks but yes, @AlexMoroz is right

TheZoker commented 8 years ago

@AlexMoroz Like "gotarffplod": "latest"?

AlexMoroz commented 8 years ago

yes

TheZoker commented 8 years ago

Done.

sacdallago commented 8 years ago

I'm still unhappily waiting.

Legenzoo commented 8 years ago

I tried to have a look into the package, but node gives me:

Error: Cannot find module 'gotplod'

I have started npm install with the new package.json and also npm install gotplod. Using the online test tool of npm gives me the same result: https://tonicdev.com/npm/gotplod

Error: Cannot find module 'gotplod' Error Viewer Whoops, should we have 'gotplod'? We should have every package on npm. Sometimes this error is caused by a typo in your require path, but if you think we are actually missing this package, please confirm below and we’ll do our best to fix it as soon as possible!

@TheZoker @AlexMoroz

Edit: The files are in node_modules. Don´t get the problem with require...

jsalahov commented 8 years ago

@Adiolis Fixed the problem with 'gotplod'. Download the new version.

AlexMoroz commented 8 years ago

@Adiolis try also gotarffplod

Legenzoo commented 8 years ago

@sacdallago Here you go ;) npm run refill --collection=characterPlods

But really the last thing i can do till saturday/sunday ;( Crack the whip on the others :stuck_out_tongue_closed_eyes:

sacdallago commented 8 years ago

Thanks @Adiolis . Don't worry about things I'm gonna crack open.

I'm even upsetter now that I know you had to implement this :laughing: UPSETTER!

sacdallago commented 8 years ago

https://api.got.show/api/plod

Thanks. @Adiolis .

gyachdav commented 8 years ago

Thank @Adiolis

sacdallago commented 8 years ago

Yes. Unfortunately the odyssey is not over.

@Adiolis is not gonna work on this, so forget about him now. @kordianbruck or @togiberlin please do this.

What @Adiolis has done seems to be half of the job, When I run the https://api.got.show/api/plod I only get the predictions of one of the two predictors. I should get predictions for every character twice, depending on the predictor used. So I guess @Adiolis has only implemented the first package he found.. which is alright because basically you now know exactly what you have to do.

Legenzoo commented 8 years ago

@sacdallago Basically duplicate the same thing for Group7/npm.js ? That are 5-10min. i can borrow. I am only a bit unsure what values the property algorithm in the charactersPlod model should get.

Legenzoo commented 8 years ago

@nicoladesocio @konstantinos-angelo @dan736923 @AlexMoroz When doing var plods = gotarffplod.init().getAllCharPredictions(); i get:

Error: ENOENT: no such file or directory, open './prediction.json' at Error (native) at Object.fs.openSync (fs.js:584:18) at Object.fs.readFileSync (fs.js:431:33) at module.exports.init (/Applications/XAMPP/xamppfiles/htdocs/got/node_modules/gotarffplod/npm.js:15:23) at Object.module.exports.updateArffPlods (/Applications/XAMPP/xamppfiles/htdocs/got/app/controllers/filler/characters.js:310:33) at /Applications/XAMPP/xamppfiles/htdocs/got/app/controllers/filler/characters.js:267:36 at /Applications/XAMPP/xamppfiles/htdocs/got/node_modules/async/lib/async.js:52:16 at done (/Applications/XAMPP/xamppfiles/htdocs/got/node_modules/async/lib/async.js:246:17) at /Applications/XAMPP/xamppfiles/htdocs/got/node_modules/async/lib/async.js:44:16 at /Applications/XAMPP/xamppfiles/htdocs/got/app/controllers/filler/characters.js:264:25 at /Applications/XAMPP/xamppfiles/htdocs/got/app/controllers/filler/characters.js:255:17 at model. (/Applications/XAMPP/xamppfiles/htdocs/got/nodemodules/mongoose/lib/document.js:1824:20) at next (/Applications/XAMPP/xamppfiles/htdocs/got/node_modules/hooks-fixed/hooks.js:89:34) at fnWrapper (/Applications/XAMPP/xamppfiles/htdocs/got/node_modules/hooks-fixed/hooks.js:186:18) at /Applications/XAMPP/xamppfiles/htdocs/got/node_modules/mongoose/lib/model.js:292:13 at /Applications/XAMPP/xamppfiles/htdocs/got/node_modules/mongoose/lib/model.js:227:5

in node_modules i have the gotarffplod dir with also the "missing" prediction.json...

If this is fixed, i am done.

nicoladesocio commented 8 years ago

The file prediction.json should be insidie the directory where the program runs, in this way it will be found

And init does not return the object, you need to separate the calls

var p= require('./npm.js);
p.init();
var chars = p.getAllCharPredictions();
console.log(chars['edmure tully']);

I'll add this example and the documentati in in npm.js. In my environment it works

Legenzoo commented 8 years ago

@nicoladesocio I am pretty sure, i don´t get this right, but to my understanding i need to copy all your files from node_modules into the base dir, what is not the idea of npm packages.

Error: ENOENT: no such file or directory, open './attribute_contribution.json'

Edit: Okay, all jsons are sufficient, but for me it is strage that i have to copy them to my base dir. I can´t even put them into some subdir, because it is hardcoded in the package, which i should not change.

@sacdallago

nicoladesocio commented 8 years ago

no, it should be enough to copy only predictions.json and attribute_contribution.json.

Otherwise I can modify the code, if it's sure that you find the files under './node_modules/gotarffplod'

PS: I am creating the documentation on the wiki now

Legenzoo commented 8 years ago

@nicoladesocio top.json has to be copied, too. @sacdallago What do you think?

Legenzoo commented 8 years ago

@sacdallago npm run refill --collection=characterPlods (but you have to copy the jsons from the node_modules/gotarffplod ; see above)

@mina-zaki algorithm is set to gotarffplod or gotplod depending on the package used.

sacdallago commented 8 years ago

This is absolutely not the way to import npm packages, needs to be fixed now. @AlexMoroz you had to package this project so please make it happen. @TheZoker you can help on the A side should @AlexMoroz run into trouble.

EDIT: if the idea behind having the predictions file somewhere else was reusability, then:

  1. You should have had a documentation long before the deadline.
  2. You should offer the default prediction and then eventually a way to override it with a config parameter specifying where the predictions file is when initing the package.

And this is CS 101, not javascript, or webpack or god knows what.

sacdallago commented 8 years ago

I'm amazed that this still needs to be done at this stage.

jsalahov commented 8 years ago

@nicoladesocio This happens because you use relative paths in your npm.js, when calling

var json_attr = fs.readFileSync('./attribute_contribution.json','utf8');

Instead use path module (which is builtin node module, you just have to require/import) to resolve the path of the file:

var json_attr = fs.readFileSync(path.resolve(__dirname,'attribute_contribution.json'),'utf8');

Root folder changes when package is imported, so paths realtive to root won't work

nicoladesocio commented 8 years ago

@CavidSalahov thanks, I was looking for this solution

@Adiolis it's done, you should now be able to use the package without copying the json files

sacdallago commented 8 years ago

No. The packe was not deployed to npm

AlexMoroz commented 8 years ago

Package deployed.

sacdallago commented 8 years ago

Ok, I've updated the thing but now comes another observation:

why is there an imbalance towards the number of predicted characters with one alg and with the otherone? Is this wanted @goldbergtatyana ?

Look at the output of the call: https://api.got.show/api/plod/

If I filter by gotplod I get 1946 hits, if I filter per gotarffplod I get 174.

Legenzoo commented 8 years ago

@sacdallago Jep, i have noticed many characters with Plod unknown. I skipped them.

sacdallago commented 8 years ago

Thx @Adiolis , let's wait for the answer of @nicoladesocio @goldbergtatyana or anyone involved in the gotarffplod.

It might as well be design, but it's good to know also for @mina-zaki who has to implement this in the froentend

goldbergtatyana commented 8 years ago

@sacdallago @Adiolis very well spotted! this should absolutely not be. The number of characters predicted by Group 6 (gotplod) is 1946 - which is correct, while the number of characters predicted by Group 7 (gotarffplod) is 1939 - this number is wrong.

sacdallago commented 8 years ago

@nicoladesocio @konstantinos-angelo @dan736923 @AlexMoroz fix?

goldbergtatyana commented 8 years ago

@s-feng apparently wrong prediction results got delivered to Group A. Can you please talk to @nicoladesocio @konstantinos-angelo @dan736923 @AlexMoroz to fix it by providing results of test 5? Thanks so much!

dan736923 commented 8 years ago

@goldbergtatyana This is because I removed some characters with invalid dateOfBirth/Death attributes (e.g. 298299) in test5. (Which also would have messed up age attribute) If you think it is useful, I can make another run including those characters and upload new JSON files.

AlexMoroz commented 8 years ago

@dan736923 but now only 174 characters have plod

goldbergtatyana commented 8 years ago

thanks @AlexMoroz for running the test.

@dan736923 but you used all of these features for the prediction of plods for 1939 characters. so, yeah, we do need plod predictions for all of them!

TheZoker commented 8 years ago

@AlexMoroz Do you need help with anything?