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

Refacto py4cast start #102

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

clemoule
Copy link

implementation of :

  • a cli_test.py for executing the lightning cli
  • a config_cli_test.yaml for parsing cli arguments
  • a dummy.py which propose a standard structure for lightningmodule and datamodule
  • a utils.py in which are created utils class that are specialized in one task each. The idea is to create function and override methods methodically during the execution of lightningmodules. (ex: class dummylightningmodule(utils1, utils2, pl.LightningModule) (--> see method resolution order for more details)

def cli_main(args: ArgsType = None):
cli = LightningCLI(
MAELightningModule,
DummyDataModule,
Copy link
Author

Choose a reason for hiding this comment

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

pas encore de autoencoderDataModule donc pour l'instant comportement encoder-decoder (x!=y) et pas autoencoder(x=y).

from py4cast.datasets import get_datasets

# A SUPPRIMER
from py4cast.datasets.base import TorchDataloaderSettings
Copy link
Author

Choose a reason for hiding this comment

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

A terme, enlever TorchDataLoaderSettings -> modifier base.py

@cbovalo
Copy link
Collaborator

cbovalo commented Nov 14, 2024

@clemoule Is this PR ready for review or is it a draft ?
BTW, after a quick review, I find the code would be more readable.

@clemoule
Copy link
Author

@clemoule Is this PR ready for review or is it a draft ? BTW, after a quick review, I find the code would be more readable

The code can be executed but i need to verify the plot/log method. We will discuss of the way to redesign the trainers, parsers, lightningmodule and datamodule this afternoon. So this is more of a draft even though it works. We will be set this afternoon.

@clemoule
Copy link
Author

storing functions in classes (ex: PlotLightningModule) is a bit of over engineering but it helps to group functions and methods by purpose. It makes the code easier to map and understand => quicker to pick up the code => quicker to implement additions to the code and quicker to tinker

@clemoule clemoule marked this pull request as draft November 14, 2024 10:47
@cbovalo
Copy link
Collaborator

cbovalo commented Nov 14, 2024

I think it's also a good thing to decouple computing metrics and making plots.
I'll make some comments on the code if improvement is needed.

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.

2 participants