neuroailab / tfutils

Utilities for working with tensorflow
MIT License
25 stars 8 forks source link

`loss_per_case_func` API #34

Closed chengxuz closed 7 years ago

chengxuz commented 7 years ago

Hi, @qbilius, I think in the function "alexnet" in model.py, the dropout rate should be determined by the "train" flag. Is that right?

qbilius commented 7 years ago

You're right. This for lost at some point over multiple edits. Thanks, I'll fix it.

On Wed, Jan 4, 2017, 19:36 Chengxu Zhuang notifications@github.com wrote:

Hi, @qbilius https://github.com/qbilius, I think in the function "alexnet" in model.py, the dropout rate should be determined by the "train" flag. Is that right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/neuroailab/tfutils/issues/34, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMaMSJdZhQmd4qnDGKnnpHwCV8pDFISks5rPDsagaJpZM4LbOUx .

chengxuz commented 7 years ago

Hi, Jonas, @qbilius , besides this, I think in the newest version of tensorflow, the loss function currently used in training alexnet (tf.nn.sparse_softmax_cross_entropy_with_logits) no longer takes positional parameters (see https://github.com/tensorflow/tensorflow/blob/master/tensorflow/g3doc/api_docs/python/functions_and_classes/shard4/tf.nn.sparse_softmax_cross_entropy_with_logits.md for details). So the tutorial can not work with the newest tensorflow with the error message:

ValueError: Only call `sparse_softmax_cross_entropy_with_logits` with named arguments (labels=..., logits=..., ...)

Can you also help fix this?

Thanks!

chengxuz commented 7 years ago

Hi, @qbilius , I need to reopen this. The change you made to utils.py regarding to this issue:

    loss = loss_per_case_func(logits=outputs, labels=labels, **loss_func_kwargs)

will have some problems as you can imagine, not every loss functions have the parameters named "logits" and "labels". I think this line should be the original line. And I think that the modification toward this issue should be outside tfutils, but inside the train_alexent.py script (the way I used in my commit).

qbilius commented 7 years ago

I was thinking about that too. My thinking was to impose this (logits and labels) as the API, and if you happen to use a loss function without one of them, then you write your own little function outside of tfutils. I'm very much against the idea that such a common loss function as cross-entropy would require external functions to work.

One other possibility would be to allow any tf loss function as an input in params, and just internally make the API uniform.

chengxuz commented 7 years ago

Another possibility is introducing "loss_func_params". For example, "loss_func_params:{'_outputs': 'logits', '_labels': 'labels'}" will be something we can use for cross-entropy loss function.

qbilius commented 7 years ago

That's an interesting possibility as well!

chengxuz commented 7 years ago

OK, I will try to implement and test that.

qbilius commented 7 years ago

If you don't need this functionality urgently, I'd prefer not doing it or doing it on a branch. It's not a good idea to keep changing API, so I'd rather talk to @yamins81 to see if he has preferences / better ideas.

yamins81 commented 7 years ago

This is important, as I agree want to lock down the API, but we almost immediately need the functionality of having loss functions that will take different parameter names. Let me think about a solution.

qbilius commented 7 years ago

Just FYI, I take back my previous statement "I'm very much against the idea that such a common loss function as cross-entropy would require external functions to work." I think some middle-ground could be found here.

yamins81 commented 7 years ago

Agreed. My computer is about to run out of batters and I left my Japanese plug converter in the conference hall ... so I'm going to have to sign off for the night... Maybe you guys can think of a better solution here.

chengxuz commented 7 years ago

Already fixed.