Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Good First Issue][NNCF]: Replace common tensor_statistics by experimental tensor_statistics #3041

Open
kshpv opened this issue Oct 28, 2024 · 12 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@kshpv
Copy link
Collaborator

kshpv commented Oct 28, 2024

Context

Historically, NNCF has maintained two sets of tensor statistics: the old tensor statistics and the new tensor statistics. The new tensor statistics, located in the experimental/common/tensor_statistics directory, offer improved functionality and better performance. However, the codebase still contains references to the old tensor statistics, leading to redundancy and potential confusion.

What needs to be done?

  1. Search the codebase for all instances where the old tensor statistics are being used. This includes algorithms, tests, and any utility functions.
  2. Modify the identified algorithms to utilize the new tensor statistics from experimental/common/tensor_statistics.
  3. Update the corresponding tests to ensure they are compatible with the new tensor statistics.
  4. Replace the contents of common/tensor_statistics/collectors with the corresponding implementations from experimental/common/tensor_statistics.
  5. Once all references to the old tensor statistics have been updated, remove the experimental/common/tensor_statistics.
  6. Remove common/tensor_statistics/reduction if it is not needed anymore.

In the end, there should be no experimental/common/tensor_statistics.

Example Pull Requests

#2117

Resources

Contact points

@kshpv

@kshpv kshpv added the good first issue Good for newcomers label Oct 28, 2024
@olegkkruglov
Copy link

.take

Copy link

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@github-project-automation github-project-automation bot moved this to Contributors Needed in Good first issues Oct 28, 2024
@alexsu52 alexsu52 moved this from Contributors Needed to Assigned in Good first issues Oct 28, 2024
@AHB102
Copy link

AHB102 commented Oct 28, 2024

@kshpv So basically functionally replace everything in old files(common) with the new files(Experimental) and then delete the new files(Experimental) ?

@kshpv
Copy link
Collaborator Author

kshpv commented Oct 29, 2024

@AHB102, you also need to be sure that all tests passed and no regression is introduced

@AHB102
Copy link

AHB102 commented Oct 29, 2024

@kshpv Great! I've reviewed the example and will create a PR for a single-file change. We can go from there

@kshpv
Copy link
Collaborator Author

kshpv commented Oct 29, 2024

@AHB102 I am really sorry, but @olegkkruglov already picked this GFI. I suggest you to pick another one.
We have many things to do. I think we probably open new GFIs in the following days.
If you are interested I can ping you. What do you say?

@AHB102
Copy link

AHB102 commented Oct 29, 2024

@kshpv Yup no problem 😄

@mlukasze
Copy link

mlukasze commented Nov 6, 2024

hey @olegkkruglov - do you need any support or you may not have a time to finish the task?

@olegkkruglov
Copy link

hey, the work is in progress. sorry, I didn't have enough time to finish. I think I will be able to do it during this weekend, is it ok?

@mlukasze
Copy link

mlukasze commented Nov 7, 2024

of course :)

@kshpv
Copy link
Collaborator Author

kshpv commented Nov 28, 2024

Hi @AHB102 #3120 - this is probably for you :)

@AHB102
Copy link

AHB102 commented Nov 29, 2024

@kshpv By the time I looked, someone else had taken the issue. Maybe next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Assigned
Development

No branches or pull requests

4 participants