NCAR / miles-holodec

Creative Commons Zero v1.0 Universal
1 stars 0 forks source link

Potentially wrong calculation for metric #9

Open shamuraii opened 3 months ago

shamuraii commented 3 months ago

https://github.com/NCAR/miles-holodec/blob/cbb6e7e80be73cca3e9e5dc1405c8830d6c05d5c/holodec/trainer.py#L117

I'm not positive, but judging off the other loss/metric calculations I think the second argument might want to be y_pred_depth * y_pred_mask instead of using y_part_mask again. If it is right though I wouldn't mind an explanation what is going on either, thanks!

shamuraii commented 3 months ago

Have stared at it a bit longer and I think I get what is happening here so it is probably correct. Feel free to still confirm / close the issue

trisodium10 commented 3 months ago

In some ways your comment isn't wrong... It's actually supposed to gradually transition from using y_part_mask to using y_pred_mask. In my most recent PR, the loss metric got more complicated. I added IntersectedMAE and IntersectedMSE.
https://github.com/NCAR/miles-holodec/blob/cbb6e7e80be73cca3e9e5dc1405c8830d6c05d5c/holodec/losses.py#L295

The complication to lightning is that these need to take four arguments instead of just two. Maybe the lightning implementation means we need to use **kwargs or *args as inputs instead of explicitly defining them?

shamuraii commented 3 months ago

The 4-argument stuff is okay, just won't be hot-swappable with 2-arg loss functions without changing the trainer args on the line the loss is calculated. Switching to **kwargs could maybe make things more flexible but also unlikely worth the effort to add and make things more complicated. It won't be difficult to switch losses even so