LucjanJanowski / translator-to-suJSON

Read subjective experiment data to suJSON format.
MIT License
2 stars 1 forks source link

Implement the export functionality #14

Open Qub3k opened 4 years ago

Qub3k commented 4 years ago

Implement this in the following function: https://github.com/LucjanJanowski/translator-to-suJSON/blob/164c9f268d881d68c5f2fab83fee3738e909e871/sujson/_sujson.py#L378

Qub3k commented 4 years ago

@awro1444 provided the first implementation. Now @Qub3k will test it.

Qub3k commented 4 years ago

@awro1444 I went through your code. You did a good job implementing the export functionality! 💪

Below please find code fragments with six TODOs that I added for you to address. Regarding TODO 3. please note that pickle can directly serialise the self.sujson object as a whole. This is what we really want to do here so let's go for it!

Naturally, if you have any questions please do not hesitate to ask them (preferably here).

https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/_sujson.py#L385 https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/_sujson.py#L391 https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/_sujson.py#L394 https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/_sujson.py#L399 https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/_sujson.py#L405 https://github.com/LucjanJanowski/translator-to-suJSON/blob/cfbf5f2c3dfcf66c6696d177c7d61943b4b34574/sujson/__main__.py#L133

Qub3k commented 4 years ago

I am in the process of the final review. After that I will merge pull reuqest #16.

Qub3k commented 4 years ago

@awro1444 Here are the TODOs that I have added in response to your recent changes. Please address them. 😌

https://github.com/LucjanJanowski/translator-to-suJSON/blob/92b67cc2bd4af7efc27bb37edd276fc548ca9339/sujson/__main__.py#L127

https://github.com/LucjanJanowski/translator-to-suJSON/blob/92b67cc2bd4af7efc27bb37edd276fc548ca9339/sujson/__main__.py#L137

Qub3k commented 4 years ago

I have concerns regarding the sujson._sujson.Sujson.pandas_export function (see below). I think it will not work if we use it on this representative suJSON file.

https://github.com/LucjanJanowski/translator-to-suJSON/blob/ff87401cfb501394aa37a7a0e6ca2b2a691480ee/sujson/_sujson.py#L383

The problem comes from the possibility of more than one score being connected with a single trial. For example, a study participant may be asked to score three images, all in one trial. This generates three scores per trial. Furthermore, the current implementation uses a risky strategy of finding subject ID related with a given score. If we process a file where there is a mismatch between the number of trials and scores, the code will fail.

I suggest to fix these shortcomings before we proceed further. I added a TODO note in the code related with this problem (see the lines below).

https://github.com/LucjanJanowski/translator-to-suJSON/blob/bb1f56090cff5ec3d889998a354832d5b66ad012/sujson/_sujson.py#L384-L386

Please note that I also propose to slightly change the target DataFrame format to make the implementation easier. I describe the new format in this comment: https://github.com/LucjanJanowski/translator-to-suJSON/issues/15#issuecomment-627998489

@awro1444 can you please address this?