mit-quest / necstlab-damage-segmentation

MIT License
5 stars 6 forks source link

Issue 75 upgrade tfversion #84

Closed CarolinaFurtado closed 3 years ago

CarolinaFurtado commented 3 years ago

Decided to separate the tf version upgrade from the ubunto and drivers upgrade.

Upgraded the tensorflow version. The only issue with the metric names. Before, loss was added by hand, with tf2.3, there is no need to do that.

Verifications:

tf2.3 provides repeatable results? Yes

image

tf 2.1 and tf 2.3 provide the same results? Very similar but not equal:

image

TRAIN:

Fixed the metrics mosaic by removing loss from metric_names. Results don't match exactly because we don't have repeatability between 2.1 and 2.3.

tf.2.1

image

tf.2.3

image

TRAIN_THRESHOLSD:

dict_results = dict(zip(metric_names, all_results)) in models.py was compiling two lists with different dimensions. meaning the selected optimizing_result was wrong in the end. Removed loss from metric_names

tf2.3 without the modification

wrong! loss + binary_crossentropy + binary_ce_metric (off by one) {'loss': 0.0010814343113452196, 'binary_ce_metric': 3.68487532154127e-11, WRONG!!!! should all be the same, but since we are dict(zip(10 names, 9 metrics)), it ignores the last one, and the 3 first are binary cross entropy 'class0_f1_score': 0.29115191102027893, 'class0_binary_accuracy_sm': 0.9995417594909668, 'binary_crossentropy': 0.0010814343113452196, 'class0_precision': 0.33629000186920166, 'class0_binary_cross_entropy': 0.9995417594909668, 'class0_iou_score': 0.31209734082221985, - this is the optimal value. wrong here 'class0_binary_accuracy_tfkeras': 0.18490245938301086}

tf2.3 with the modification

right! loss + binary_ce_metric {'class0_f1_score': 0.31209734082221985, 'class0_precision': 0.29115191102027893, 'class0_recall': 0.33629000186920166, 'loss': 0.0010814343113452196, 'class0_binary_cross_entropy': 3.68487532154127e-11, 'class0_binary_accuracy_tfkeras': 0.9995417594909668, 'binary_ce_metric': 0.0010814343113452196, 'class0_binary_accuracy_sm': 0.9995417594909668, 'class0_iou_score': 0.18490245938301086} - this is the optimal value. ok here

TEST:

Same issue: removed loss from metric_names. Results match.

image

rak5216 commented 3 years ago

great! just to confirm for consistency, a byproduct of removing our manual addition of loss.name or loss.__name__ to metric_names is that the new tf default in tf 2.3 is that 'loss' (rather than 'binary_crossentropy' which is loss.name) is now automatically added in train model, test model, and models.py? Just want to make sure the only place that 'binary_crossentropy' (loss.name) appears now is in train model plots y axes? which is okay. the metrics .csv everywhere will read just 'loss' though

rak5216 commented 3 years ago

only additional change i would suggest is to add a comment in requirements.txt like tensorflow-gpu # use version >= 2.2

CarolinaFurtado commented 3 years ago

ok, so for train, this lines:

        if metric_name == 'loss' and hasattr(compiled_model.loss, '__name__'):
            axes[counter_m, counter_n].set_ylabel(compiled_model.loss.__name__)
        elif metric_name == 'loss' and hasattr(compiled_model.loss, 'name'):
            axes[counter_m, counter_n].set_ylabel(compiled_model.loss.name)
        else:
            axes[counter_m, counter_n].set_ylabel(metric_name)

still mean that the name will be replaced from loss to binary_crossentropy in the plots: image

However, in the metrics.csv, it will be called lossas before.

For test, the same happens, we are no longer replacing the name 'loss' with binary_crossentropy:

    metric_names = [m.name for m in compiled_model.metrics]
    with Path(test_dir, str('metrics_' + test_datetime + '.csv')).open('w') as f:
        f.write(','.join(metric_names) + '\n')
        f.write(','.join(map(str, results)))

the easiest way to make everything consistent would be to NOT replace the loss name anywhere. So loss would always be used instead of binary_crossentropy. We just have to replace ignore the ifsin train:

        if metric_name == 'loss' and hasattr(compiled_model.loss, '__name__'):
            axes[counter_m, counter_n].set_ylabel(compiled_model.loss.__name__)
        elif metric_name == 'loss' and hasattr(compiled_model.loss, 'name'):
            axes[counter_m, counter_n].set_ylabel(compiled_model.loss.name)
        else:
            axes[counter_m, counter_n].set_ylabel(metric_name)

rather have it that way? @rak5216