vDawgg / awt-pj-ss24-finding_scenes-2

MIT License
1 stars 1 forks source link

Refactor create_keyframes_csv() #21

Closed vDawgg closed 5 months ago

vDawgg commented 6 months ago

Make the function return the path to the keyframes csv instead of specifying it as a parameter.

liminm commented 6 months ago

@vDawgg Does this mean it should not be specified as an input parameter at all? In that case the path would need to be hardcoded, whereas I'd prefer to have at least the option to specify a different path for the output.

Returning the path itself is of course not an issue.

vDawgg commented 6 months ago

If we reuse the keyframes csv I dont see a reason why we should pass this as a param.

liminm commented 6 months ago

I can't quite understand why the csv parameter should be removed as input. The function itself creates the keyframes.csv first so its not reused per se, and the path is needed to specify the save destination. Otherwise it's really just a hardcoded path right inside the function which I would like to avoid for stylistic reasons. I suppose another option is a global variable

vDawgg commented 6 months ago

Dismiss my previous comment here. My point for this issue generally was that the behavior here should be relatively consistent. In the other util functions we follow a predefined structure (videos are written to videos, video segments get their own named directory by default). I would opt to create a directory with the original videos name and a 'keyframe suffix' so we dont have to specify it everytime. Passing the output path as a parameter is also completely valid, we just dont do this anywhere else.