Split aoi dataframe by chosen criteria

[izaskr]

Description

The change introduces a splitting function for the aoi dataframe in the TextStimulus class. This allows for the aoi file to be split according to some criteria, e.g. pages.

Implemented changes

Insert a description of the changes implemented in the pull request.

• Add a method to split the aoi dataframe by some criteria into a list of dataframes.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change is or requires a documentation update

How Has This Been Tested?

• Test test_text_stimulus_splitting checks if the number of resulting dataframes equals the expected length.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2d48f2f) to head (652d0b0).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #879   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           74        74           
  Lines         3372      3374    +2     
  Branches       594       594           
=========================================
+ Hits          3372      3374    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dkrako]

I'm not sure if the Textstimulus.aois attribute should change types.

Before calling split_aois_by() the type is pl.DataFrame afterwards it's a list of dataframes. It would be nice to have just one type for the aois field.

Also, when you just split the data, you won't know the splitting criteria anymore, i.e. you won't know which dataframe is which belonging to which page.

Can you include a short code example where and how you expect the method to be used? It is probably part of a greater plan.

[dkrako]

avoid having the same id for different test parametrizations.

you could use something like: toy_text_1_1_aoi_sep

[dkrako]

just a few things about the testing and we're ready to go!

[dkrako]

thanks a lot! just some more details on the tests and we're ready to merge

[dkrako]

Thank you @izaskr for your work!

Unfortunately I have missed a crucial line during my review two weeks ago as the split() method doesn't return list[TextStimulus].

I have posted a small code snippet how to change the logic accordingly, shouldn't be much work.

This change would also need an additional test: please check that all fields (e.g. aoi_column, width_column, etc.) in the returned TextStimulus splits are equal to the original TextStimulus.

[dkrako]

I must have missed this in my review two weeks ago.

this line returns a list of polars.DataFrame not a list of TextStimulus objects.

You should probably do something like this:

return [
    TextStimulus(
        aois=df,
        aoi_column=self.aoi_column,
        width_column=self.width_column,
        ...,  # remaining fields
        page_column=self.page_column,
    )
    for df in self.aois.partition_by(by=by, as_dict=False)
]

[dkrako]

@SiQube I am confused why mypy didn't complain here, as I would expect a type mismatch. I am clueless. Do you have an explanation? I am a bit worried that there's something wrong in our CI.