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

feat: get rid off output and input conversion for user #732

Open
Gerhardsa0 opened this issue May 6, 2024 · 5 comments
Open

feat: get rid off output and input conversion for user #732

Gerhardsa0 opened this issue May 6, 2024 · 5 comments
Labels
enhancement 💡 New feature or request

Comments

@Gerhardsa0
Copy link
Contributor

Is your feature request related to a problem?

Right now the user needs to set Input and Output conversion, but its always the same type of conversion.

Desired solution

Create one conversion, which the user sees.

Possible alternatives (optional)

No response

Screenshots (optional)

No response

Additional Context (optional)

No response

@lars-reimann
Copy link
Member

Is this different from #656?

@Gerhardsa0
Copy link
Contributor Author

Is this different from #656?

@Marsmaennchen221 and I were thinking about removing Output and Input Conversion and generalizing it, to a Conversion.
The problem with that is, that for Time Series, I like that I have two separate interfaces for defining parameters, like window_size, forecast_horizon and prediction_name.

@lars-reimann
Copy link
Member

lars-reimann commented May 13, 2024

I'm wondering whether the output conversion is really needed. We should already know from the dataset we get as input what the desired shape of the output is.

Likewise for the input conversions, there's a strong overlap to datasets, as we discussed:

  • InputConversionTable needs no extra arguments.
  • InputConversionImage needs the image size, which is already in the dataset.
  • InputConversionTimeSeries needs the window size and forecast horizon. We could also move these to the dataset.

Edit 1: The input conversion can be helpful, though, to indicate the input type of fit and the input and output type of predict. We could keep these completely empty and just use them to set the type parameters of the model correctly.

Edit 2: We also get this information when we fit, however, based on the given dataset. Since fit returns a new NN instance, it could specify its type parameters then. For the unfitted model, we can just leave the type parameters on Any. This means, we'd need a common superclass Dataset for all datasets, with at least two type parameters (input and output type).

@Marsmaennchen221 @Gerhardsa0 @sibre28

@Marsmaennchen221
Copy link
Contributor

Edit 2: We also get this information when we fit, however, based on the given dataset. Since fit returns a new NN instance, it could specify its type parameters then. For the unfitted model, we can just leave the type parameters on Any. This means, we'd need a common superclass Dataset for all datasets, with at least two type parameters (input and output type).

If we do this, what is the point of having a __init__ anyway? At least for Image NNs, the image size is needed to build the internal PyTorch model, which we do so far in the __init__ method. If we move this to the fit method, I don't see any real point of having an unfitted model, as the only thing we can do with it is to fit the model.

@lars-reimann

@lars-reimann
Copy link
Member

lars-reimann commented May 13, 2024

  • We could compose several (untrained) NNs into an ensemble or data processing pipeline, which can then be trained.
  • We define the layers of the model in __init__. This is consistent with the classical models, where we define hyperparameters in __init__ and differentiate between unfitted and fitted models.
  • Hyperparameter optimization #264 can still be integrated into __init__, where we could specify several architectures to try.

At least for Image NNs, the image size is needed to build the internal PyTorch model, which we do so far in the __init__ method.

Does this already initialize the weights of the model? If so, it would be considerably more efficient to do this in fit, since we must otherwise create a clone of these to prevent mutation.

lars-reimann added a commit that referenced this issue May 20, 2024
Closes partially #732

### Summary of Changes

Output conversions are the exact inversion of the input conversion, so
there is no need to specify them again. Now, a neural network only takes
an input conversion and a list of layers. This also gets rid of several
errors that could occur if input and output conversions did not fit
together.

In a later PR, the input conversion will also be removed, since they
mirror datasets.

---------

Co-authored-by: megalinter-bot <[email protected]>
lars-reimann pushed a commit that referenced this issue May 29, 2024
## [0.26.0](v0.25.0...v0.26.0) (2024-05-29)

### Features

* `Table.count_row_if` ([#788](#788)) ([4137131](4137131)), closes [#786](#786)
* added method to load pretrained models from huggingface ([#790](#790)) ([dd8394b](dd8394b))
* infer input size of forward and LSTM layers ([#808](#808)) ([098a07f](098a07f))
* outline around dots of scatterplot ([#785](#785)) ([ee8acf7](ee8acf7))
* remove output conversions ([#792](#792)) ([46f2f5d](46f2f5d)), closes [#732](#732)
* shorten some excessively long names ([#787](#787)) ([1c3ea59](1c3ea59)), closes [#772](#772)
* specify column names in constructor of table transformers ([#795](#795)) ([69a780c](69a780c))
* store window size and forecast horizon in dataset ([#794](#794)) ([f07bc5a](f07bc5a))
* string operations on cells ([#791](#791)) ([4a17f76](4a17f76))

### Bug Fixes

* handling of boolean columns in column statistics ([#778](#778)) ([f61cceb](f61cceb))
* sort x values of line plot ([#782](#782)) ([74d8649](74d8649))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 💡 New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

3 participants