Skip to content

Conversation

@morand-g
Copy link
Collaborator

@morand-g morand-g commented Nov 8, 2024

📝 Changelog

In malpolon/models/standard_prediction_systems.py: Added a generic RegressionSystem

  • Updated hardcoded references to classification
  • Removed classification-specific default metric
  • Updated loss creation so it can be set from the config file (cf. next section)

In malpolon/models/utils.py:

  • Updated argument expected type to allow loss creation from mapping (from cfg file)
  • Added condition to trigger loss creation from mapping.

	modified:   malpolon/models/standard_prediction_systems.py
	modified:   malpolon/models/utils.py
return loss
raise ValueError(f"Loss must be of type nn.modules.loss. "
elif isinstance(loss, str):
return eval(loss)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are aiming at removing all calls to eval() for security purposes. Please take a look at functions check_optimizer(), check_scheduler() or check_metrics(), on our new branch "no-more-evals" to get an understanding at what a similar check_loss() function should look like

lr: float = 1e-2,
weight_decay: float = 0,
metrics: Optional[dict[str, Callable]] = None,
task: str = 'regression_multilabel',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Argument not used


class RegressionSystem(GenericPredictionSystem):
"""Regression task class."""
def __init__(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some arguments (optimizer kwargs) are not used because incompatible with the proposed default optimizer. I suggest replacing them with the default optimizer's; or simply removing them.

Please update the docstring accordingly too

Copy link
Collaborator

@tlarcher tlarcher left a comment

Choose a reason for hiding this comment

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

The RegressionSystem class might be redundant with #66 but as the later is scheduler to take more time to be fully completed, and considering this PR is a smaller I think I'll merge this one 1st.

Also, it raises the quesiton of updating check_loss() in a similar fashion as has been done with the other "check" functions, which is good.

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