Skip to content

Conversation

@zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Mar 19, 2025

This is a perfect task to ask AI agent to work on.

@github-actions
Copy link

github-actions bot commented Mar 19, 2025

Review updated until commit 4fe0d8d

Description

  • Replaced c10::irange with std::views::iota for range-based loops

  • Included <ranges> header in multiple files

  • Updated loop indices to use std::views::iota for better readability and modern C++ practices


Changes walkthrough 📝

Relevant files
Enhancement
15 files
compute_at.cpp
Replaced c10::irange with std::views::iota                             
+2/-1     
contiguity.cpp
Replaced c10::irange with std::views::iota                             
+3/-2     
evaluator_common.cpp
Replaced c10::irange with std::views::iota                             
+3/-2     
expr_evaluator.cpp
Replaced c10::irange with std::views::iota                             
+3/-2     
expr_simplifier.cpp
Replaced c10::irange with std::views::iota                             
+14/-14 
base_nodes.cpp
Replaced c10::irange with std::views::iota                             
+2/-1     
mutator.cpp
Replaced c10::irange with std::views::iota                             
+4/-3     
polymorphic_value.cpp
Replaced c10::irange with std::views::iota                             
+3/-2     
predicate_compute.cpp
Replaced c10::irange with std::views::iota                             
+3/-2     
translate_no_reduction_matmul_to_mul_squeeze.cpp
Replaced c10::irange with std::views::iota                             
+5/-4     
segmentation.cpp
Replaced c10::irange with std::views::iota                             
+6/-4     
translation.cpp
Replaced c10::irange with std::views::iota                             
+4/-3     
executor.cpp
Replaced c10::irange with std::views::iota                             
+16/-15 
transform_rfactor.cpp
Replaced c10::irange with std::views::iota                             
+4/-3     
transform_view.cpp
Replaced c10::irange with std::views::iota                             
+9/-5     

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
⚡ Recommended focus areas for review

Possible Issue

The change from c10::irange to std::views::iota may not be compatible with all environments, especially if the code is compiled with older versions of C++ that do not support C++20 ranges. Ensure that the build environment supports C++20 or update the code to maintain compatibility.

for (const auto i : std::views::iota(0LL, val_chains.size())) {
Possible Issue

Similar to the issue in compute_at.cpp, the change from c10::irange to std::views::iota may not be compatible with all environments. Ensure that the build environment supports C++20 or update the code to maintain compatibility.

for (const auto alloc_i : std::views::iota(0LL, alloc_domain.size())) {
Possible Issue

The change from c10::irange to std::views::iota may not be compatible with all environments. Ensure that the build environment supports C++20 or update the code to maintain compatibility.

for (size_t i : std::views::iota(0LL, inputs.size())) {

@zasdfgbnm
Copy link
Collaborator Author

!test --diff

@zasdfgbnm zasdfgbnm requested review from naoyam and rdspring1 March 19, 2025 18:17
@zasdfgbnm zasdfgbnm changed the title Change c10::irange to iota Change c10::irange to iota, part 1 Mar 19, 2025
@zasdfgbnm
Copy link
Collaborator Author

There are failures, moving to draft

@zasdfgbnm zasdfgbnm marked this pull request as draft March 19, 2025 19:13
@zasdfgbnm zasdfgbnm requested review from Copilot and removed request for naoyam and rdspring1 March 19, 2025 19:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (15)
  • csrc/compute_at.cpp: Language not supported
  • csrc/contiguity.cpp: Language not supported
  • csrc/evaluator_common.cpp: Language not supported
  • csrc/expr_evaluator.cpp: Language not supported
  • csrc/expr_simplifier.cpp: Language not supported
  • csrc/ir/base_nodes.cpp: Language not supported
  • csrc/mutator.cpp: Language not supported
  • csrc/polymorphic_value.cpp: Language not supported
  • csrc/predicate_compute.cpp: Language not supported
  • csrc/preseg_passes/translate_no_reduction_matmul_to_mul_squeeze.cpp: Language not supported
  • csrc/python_frontend/segmentation.cpp: Language not supported
  • csrc/python_frontend/translation.cpp: Language not supported
  • csrc/runtime/executor.cpp: Language not supported
  • csrc/transform_rfactor.cpp: Language not supported
  • csrc/transform_view.cpp: Language not supported

Copy link
Collaborator

@rdspring1 rdspring1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this was cursor and it was taking awhile. 😢

Prompt: "Replace c10::irange with std::views::iota"

It looks like it was searching for the c10::irange function definition to do an intelligent replace. Probably should add pytorch/c10/util/irange.h to the context.

I also tried asking for a simple string match replace and it defaulted to its original approach.

@rdspring1
Copy link
Collaborator

iota expects the types for both integer arguments to match. This is kinda annoying for the human or AI agent.

Copy link
Collaborator

@rdspring1 rdspring1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid merging with test failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants