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

Add vit isic #221

Merged
merged 14 commits into from Nov 29, 2022
Merged

Add vit isic #221

merged 14 commits into from Nov 29, 2022

Conversation

shatz01
Copy link
Collaborator

@shatz01 shatz01 commented Nov 23, 2022

  • adds vit to isic
  • makes vit default
  • adds pl_func for clearml logger
  • adds clearml logger to isic

Copy link
Collaborator

@mosheraboh mosheraboh left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good!
Few comments inline.

@@ -208,6 +277,8 @@ def run_train(paths: dict, train_common_params: dict) -> None:
lgr.info("Datamodule:", {"attrs": "bold"})

datamodule = create_datamodule(paths, train_common_params)
# datamodule.setup(stage="fit")
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed here 8e9a212

@@ -194,6 +261,8 @@ def run_train(paths: dict, train_common_params: dict) -> None:
# ==============================================================================
# Logger
# ==============================================================================
data_string = "multimodal" if multimodality else "imaging"
start_clearml_logger(project_name="SHATZ_isic_II", task_name=f"{model_type}_{data_string}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

change the project name to be an argument (set by default to None). Start clearml logger only if it's not None.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain how it knows the server address?
Is it a global configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes you make a ~/clearml.conf file and specify server address. ill dm you more details



def start_clearml_logger(
project_name: Union[str, None] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a must here? remove the None if it's must.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you referring to all of the arguments? Or just project name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

made basic args mandatory here: 31a8f07

@@ -36,6 +36,48 @@
from fuse.eval import MetricBase
from fuse.eval.metrics.utils import PerSampleData

# for clearml
from clearml import Task
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add clearml to dependency list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added to dependency in 31a8f07

@mosheraboh mosheraboh merged commit 8b12ad8 into BiomedSciAI:master Nov 29, 2022
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.

None yet

2 participants