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

Paths and file management #61

Merged
merged 10 commits into from
Nov 8, 2024
Merged

Paths and file management #61

merged 10 commits into from
Nov 8, 2024

Conversation

alex404
Copy link
Contributor

@alex404 alex404 commented Oct 31, 2024

So this does quite a fair bit of refactoring.

  • I setup the analyses in statistics.py to return dataclass instances to make things more manageable and readable.
  • I also made a push to a more pathlib style of directory management, in particular in the runner submodules.
  • Generally going for a style where the at the top of the framework command (e.g. intialize, analyze or train) all necessary variables are extracted out of the DictConfig.
  • Saving analysis results to disk was the original push for this, and that's done too.

I definitely wasn't perfectly complete or consistent with how I did this, but I think things are significantly better organized than they were before. Ideally this provides a good reference for how to factor/refactor such code in other parts of the library. Definitely open to feedback on review.x

Resolves #43

@alex404 alex404 requested a review from fabioseel October 31, 2024 11:48
@alex404
Copy link
Contributor Author

alex404 commented Oct 31, 2024

This probably seems more complicated than it is. The bulk of the changes are moving blocks of arguments around into dataclasses.

@alex404
Copy link
Contributor Author

alex404 commented Oct 31, 2024

Actually hold on just discovered a bug

@alex404
Copy link
Contributor Author

alex404 commented Oct 31, 2024

Okay seems to be fine now

Copy link
Contributor

@fabioseel fabioseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems mostly fine, just want to hear your comment thoughts on my comment in plot (moving the inner function out to module scope)
and in statistics (using the defined dataclasses already within the functions)

retinal_rl/analysis/plot.py Outdated Show resolved Hide resolved
retinal_rl/analysis/statistics.py Show resolved Hide resolved
retinal_rl/analysis/statistics.py Outdated Show resolved Hide resolved
runner/frameworks/classification/initialize.py Outdated Show resolved Hide resolved
@alex404 alex404 requested a review from fabioseel November 6, 2024 11:00
Copy link
Contributor

@fabioseel fabioseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor change to avoid the duplicate code

retinal_rl/analysis/statistics.py Outdated Show resolved Hide resolved
@alex404 alex404 requested a review from fabioseel November 7, 2024 15:56
Copy link
Contributor

@fabioseel fabioseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now :)

@fabioseel fabioseel merged commit 1dd6176 into master Nov 8, 2024
3 checks passed
@fabioseel fabioseel deleted the paths-and-file-management branch November 8, 2024 10:19
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

Successfully merging this pull request may close these issues.

Improve directory and file management
2 participants