Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some small comments regarding Code Structure and Code Formatting #3

Open
3 of 4 tasks
AKuederle opened this issue Feb 12, 2024 · 4 comments
Open
3 of 4 tasks

Comments

@AKuederle
Copy link

AKuederle commented Feb 12, 2024

Overall the code quality looks great, but here are a couple of smaller things that could be improved:

  • Use of Path instead of strings for file/folder paths -> This will solve a bunch of issues caused by the differences between operating systems. For example it would make .replace('\\', '/') obsolote in the example
  • At the moment the files are huge. It might help overall readibility to split the large files into logical chunks
  • "random" and repeated comments (For example at the beginning of the individual file)
  • Inconsistent and unusuale formatting choices (in particular the space before the opening bracket of a function) can be confusing. I would highly recommend to stick to official code guidelines and enforce them by using ruff or black
@Aminsinichi
Copy link
Owner

Aminsinichi commented Feb 28, 2024

Thank you @AKuederle for these suggestions. I have a few questions to make sure I grasp this correctly:

  • For the Path suggestion, do you mean doing this?

In the beginning, define the path like this:

from pathlib import Path 

pp = "P25"
path = Path("COPY-YOUR-ADDRESS-HERE")
conditions = ['sitting', 'arithmetic', 'recovery']
devices = ["kyto", "empatica", "vu"]
criterion = "vu"

Then, I also need modify the functions slightly to incorporate this. For instance, in the individual.import_data() function, instead of path_vu = path + pp + '_' + "vu" + '.txt', I modify it to path_vu = path / f"{pp}_vu.txt".

Is this what you are suggesting to be done?

  • Could you also elaborate on what you mean by huge files, and how I can turn them into smaller logical chunks?
  • I improved the readability of the code using ruff and black in VS code, in accordance with PEP8. Related commits: commit1, and commit2.

@AKuederle
Copy link
Author

Regarding the path thing:

I would usggest to use Path internally everywhere and all functions that can take a file-path or directory as input, to support both str and Path. So when the user passes as str you convert it to path. This makes sure that you don't have to deal with OS differences internally and you allow the user to also benefit from using Path (but they don't have to)

@AKuederle
Copy link
Author

Regarding huge files: While this is a matter of preference, most people structure their functions in multiple smaller files based on categories (e.g. plotting, utils, io, loading, stats, ...). And then depending on the project, the relevant methods are reexported in user facing files to separate the internal structure from the import structures for users. For example see scikit-learn: https://github.com/scikit-learn/scikit-learn/tree/main/sklearn/cluster

Here the implementaiton is split in smaller files to easier find things, but then user facing functions are re-imported in the top-level init to provide more convenient import syntax

@Aminsinichi
Copy link
Owner

  • Thank you! path has been replaced everywhere, internally in each function, with Path; related commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants