tnc-br / ddf-isoscapes

3 stars 0 forks source link

Implement boosting + show results #165

Open VimWizardVersace opened 1 year ago

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

VimWizardVersace commented 1 year ago

In the file:

Invert the normalization on our outputs, and add kriging predictions as
    # constants so the network only predicts the residuals.
    mean_scaler = sp.label_scaler.named_transformers_['mean_std_scaler']
    unscaled_mean = mean_output * mean_scaler.var_ + mean_scaler.mean_
    untransformed_mean = krig_mean + unscaled_mean

Don't we also want to inverse transform the kriging feature like we do for the unscaled_mean i.e. unscaled_krig_mean? From what I see we call load_and_scale on the kriging mean and variance columns.

Also please use the rmse calculation from ddf common that's now checked in

This isn't necessary if I never applying any sort of scaling to the kriging features, right?

rothn commented 1 year ago

In the file:

Invert the normalization on our outputs, and add kriging predictions as
    # constants so the network only predicts the residuals.
    mean_scaler = sp.label_scaler.named_transformers_['mean_std_scaler']
    unscaled_mean = mean_output * mean_scaler.var_ + mean_scaler.mean_
    untransformed_mean = krig_mean + unscaled_mean

Don't we also want to inverse transform the kriging feature like we do for the unscaled_mean i.e. unscaled_krig_mean? From what I see we call load_and_scale on the kriging mean and variance columns.

Also please use the rmse calculation from ddf common that's now checked in

This isn't necessary if I never applying any sort of scaling to the kriging features, right?

As we previously discussed, this inverse transform is a potentially helpful thing we can do to help our small model learn better. With the inverse transform, the model only has to output a range in approximately [-3, 3] rather than directly predicting the targets. We never want to apply scaling to the Kriging features, either way, because we want to use properly-scaled values in the KL loss function rather than normalized outputs.

Since we've gone back and forth on this a few times, maybe it would be helpful to cover this in a pair programming session. Would you be open to doing that tomorrow? I'll put something on your calendar.

VimWizardVersace commented 1 year ago

https://screenshot.googleplex.com/6A8LoqXghQA5hxQ

columns_to_keep is just used as an argument to filter columns in the dataframe and doesn't affect the order of columns in the dataframe.

The test in the model code does raise an exception if the columns are out of order.

I change the order here to make it more intuitive to read.

VimWizardVersace commented 1 year ago

In the file:

Invert the normalization on our outputs, and add kriging predictions as
    # constants so the network only predicts the residuals.
    mean_scaler = sp.label_scaler.named_transformers_['mean_std_scaler']
    unscaled_mean = mean_output * mean_scaler.var_ + mean_scaler.mean_
    untransformed_mean = krig_mean + unscaled_mean

Don't we also want to inverse transform the kriging feature like we do for the unscaled_mean i.e. unscaled_krig_mean? From what I see we call load_and_scale on the kriging mean and variance columns.

Also please use the rmse calculation from ddf common that's now checked in

I can do this, but this would involve a pretty major refactor as I would now need to generate isoscapes as part of train_and_evaluate().

Also, some of the models we are experimenting with can not (at least yet) be used to generate isoscapes, so I would still like to fall back on this logic if needed.

rothn commented 1 year ago

In the file:

Invert the normalization on our outputs, and add kriging predictions as
    # constants so the network only predicts the residuals.
    mean_scaler = sp.label_scaler.named_transformers_['mean_std_scaler']
    unscaled_mean = mean_output * mean_scaler.var_ + mean_scaler.mean_
    untransformed_mean = krig_mean + unscaled_mean

Don't we also want to inverse transform the kriging feature like we do for the unscaled_mean i.e. unscaled_krig_mean? From what I see we call load_and_scale on the kriging mean and variance columns.

Also please use the rmse calculation from ddf common that's now checked in

I can do this, but this would involve a pretty major refactor as I would now need to generate isoscapes as part of train_and_evaluate().

Also, some of the models we are experimenting with can not (at least yet) be used to generate isoscapes, so I would still like to fall back on this logic if needed.

I do not understand this thread @erickzul

VimWizardVersace commented 1 year ago

In the file:

Invert the normalization on our outputs, and add kriging predictions as
    # constants so the network only predicts the residuals.
    mean_scaler = sp.label_scaler.named_transformers_['mean_std_scaler']
    unscaled_mean = mean_output * mean_scaler.var_ + mean_scaler.mean_
    untransformed_mean = krig_mean + unscaled_mean

Don't we also want to inverse transform the kriging feature like we do for the unscaled_mean i.e. unscaled_krig_mean? From what I see we call load_and_scale on the kriging mean and variance columns.

Also please use the rmse calculation from ddf common that's now checked in

I can do this, but this would involve a pretty major refactor as I would now need to generate isoscapes as part of train_and_evaluate(). Also, some of the models we are experimenting with can not (at least yet) be used to generate isoscapes, so I would still like to fall back on this logic if needed.

I do not understand this thread @erickzul

https://docs.google.com/spreadsheets/d/11mN27KPF0OQZ7jEJc_0IUbm9YOiQVsg9cnBD9Ou9DUI/edit#gid=1121609787

Some models use Iso_Oxi_Stack_mean_TERZER and isoscape_fullmodel_d18O_prec_REGRESSION, generated using isoscapes produced by martinelli's lab. It's 1) not clear if we have them 2) if we can continue expecting the lab to produce these isoscapes in the future.

erickzul commented 1 year ago

@VimWizardVersace For handing off the model, we know we can't use those columns from other models and they haven't illustrated yet a path forward to move with them. I would suggest we check in duplicate code to calculate RMSE for something that is not usable by the client. It can be a draft/not-checked in code, but I wouldn't send a PR for it to be in production.

rothn commented 1 year ago

I thought the data did not support this approach. Did something change?