search

pytorch / vision

Datasets, Transforms and Models specific to Computer Vision
https://pytorch.org/vision
BSD 3-Clause "New" or "Revised" License
16.13k stars 6.94k forks source link

Throw ValueError for wrong reduction in losses #6604

Closed oke-aditya closed 2 years ago

oke-aditya commented 2 years ago

As discussed in #6457

The current problem

We do not restrict the reduction mode in losses. There are only three cases possible, ["none", "mean", "sum"]. If the user passes something like "xyz" we are not throwing any error or warning.

The solution

If users aren't passing one of the valid cases and using a random string instead. It's better to throw a ValueError suggesting them to use one of the correct choice.

We had thought of Enum based solution, but to keep better consistency with Pytorch core as discussed, we are adopting the above solution.

Edits required

Need to edit losses. which are namely ciou_loss, diou_loss, giou_loss, and focal_loss

Also need to edit the relevant tests in test_ops.py

Additional Context

I think it's good first issue and some new contributor can take this :smiley:

Feel free to ping the maintainers or me if you need help. :smile:

NicolasHug commented 2 years ago

Looking at https://github.com/pytorch/vision/pull/6457/files, it looks like this is taken care of in the original PR already? Or were you referring to other losses @oke-aditya ?

datumbox commented 2 years ago

I think @oke-aditya refers to all the other losses. I believe that's a good idea but if the ticket is intended for a new contributor, it's worth listing explicitly all the places were the change needs to happen and a potential idiom we want to adopt. This will help people who are not familiar with our code-base to pick it up.

oke-aditya commented 2 years ago

Yeah I'm referring to all the other losses. I will add further details to help new contributors :smiley:

oke-aditya commented 2 years ago

Well I hope I have edited the issue and explained it in bit more detail. I haven't given away the code of what needs to be done intentionally (as I believe some new contributor should try to figure out and interact with people while fixing)

cc @datumbox

datumbox commented 2 years ago

@oke-aditya Looks great to me! Thanks for helping us attract more contributors :)

adityagandhamal commented 2 years ago

I can work on this. Would you please assign me the task?

oke-aditya commented 2 years ago

@adityagandhamal feel free to send PR and ping me as a reviewer. I can review.

Also btw how did you come to know about this issue?

adityagandhamal commented 2 years ago

I recently started using PyTorch and was browsing about, looking for anything to contribute to, and fortunately, I came upon this!

datumbox commented 2 years ago

@adityagandhamal Welcome! I've assigned the ticket to you, looking forward to your PR!

adityagandhamal commented 2 years ago

Yes. I've finished making the necessary changes to the code and am now working on the tests. Will send the PR soon.

adityagandhamal commented 2 years ago

While running a test on test_giou_loss specifically, I am running into an error AttributeError: module 'torchvision.ops' has no attribute 'generalized_box_iou_loss'

Quite strange to have encountered such an error given the loss function exists in the module

datumbox commented 2 years ago

This sounds weird. It seems like if TorchVision is not properly installed. I would start by a fresh conda environment and follow this guide. Let me know if you continue facing problems.

adityagandhamal commented 2 years ago

Yes, I've been following the same guide but nonetheless, I'll go for it again. But before I give another run at this, I'd like you to know that I did come across an error after running python setup.py develop. It was regarding Microsoft Visual C++ Build Tools. It kept on returning the same error even after installing the necessary package.

And pardon me if this sounds naive but one has to clone the forked repo into their local machine but in the guide, why is it mentioned to clone the parent repo? git clone https://github.com/pytorch/vision.git

oke-aditya commented 2 years ago

I'd like you to know that I did come across an error after running python setup.py develop. It was regarding Microsoft Visual C++ Build Tools.

Can you send detailed log of the same? Sorry I can't help much as I use linux only.

but one has to clone the forked repo into their local machine but in the guide, why is it mentioned to clone the parent repo? git clone https://github.com/pytorch/vision.git

Yeah you need to clone the fork. The guide might be written to build from source and not to contribute.

adityagandhamal commented 2 years ago

Okay, after spending some time on StackOverflow and rebooting the system, now it seems everything is good to go. Finished processing dependencies for torchvision==0.14.0a0+30b879f

@oke-aditya @datumbox I'll make the edits, run the tests, and ping you if needed. Thank you

adityagandhamal commented 2 years ago

Just to test whether torchvision is installed properly, I tried importing it only to get a windows popup error Screenshot (854)

I can't figure this out

oke-aditya commented 2 years ago

Actually you are doing quite a difficult thing. Building on windows. Much harder. WSL2 can help you though to make it easier.

Have you run

python setup.py develop ?

oke-aditya commented 2 years ago

Also I believe we should have a step by step guide for windows @datumbox most of us don't probably use windows and it's a pain point for new developers to setup.

For Linux it's very very straightforward and hardly takes an hour.

adityagandhamal commented 2 years ago

Have you run

python setup.py develop ?

Yes, I have. It ran without giving any error and returned Finished processing dependencies for torchvision==0.14.0a0+30b879f

adityagandhamal commented 2 years ago

Yes, I agree that developing on WSL is always a better option.

On WSL as well, should the same steps be followed (according to the guide) as before?

oke-aditya commented 2 years ago

Yeah WSL you can just install Ubuntu and follow the steps mentioned for Linux. It will have gcc compiler and hence your half job will be done.

On WSL I believe you will need

  1. Install miniconda
  2. Create env and install Python
  3. Install pytorch nightly. For now cpuonly would suffice.
  4. clone torchvision. Follow the contribution guide

Basically 4 is

conda install libpng jpeg
python setup.py develop
pip install flake8 typing mypy pytest pytest-mock scipy
adityagandhamal commented 2 years ago

WSL2 is great @oke-aditya! torchvision has successfully set up. No absurd errors so far. Thank you

oke-aditya commented 2 years ago

One of the nice things about good first issues is that it helps to understand what the difficulty a new contributor may face.

It's quite ok and in fact expected that a new contributor will need help in getting started and setting up. So nothing wrong about seeking help.

Keep going @adityagandhamal (btw our first name is the same :smiley:)

adityagandhamal commented 2 years ago

@oke-aditya @datumbox I have sent the PR, open to suggestions