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

Counting Coprimes Solution #4742

Closed
wants to merge 14 commits into from
Closed

Counting Coprimes Solution #4742

wants to merge 14 commits into from

Conversation

lext88
Copy link
Contributor

@lext88 lext88 commented Sep 2, 2024

Place an "x" in the corresponding checkbox if it is done or does not apply to this pull request.

  • [X ] I have tested my code.
  • [X ] I have added my solution according to the steps here.
  • [X ] I have followed the code conventions mentioned here.
    • I understand that if it is clear that I have not attempted to follow these conventions, my PR will be closed.
    • If changes are requested, I will re-request a review after addressing them.
  • [X ] I have linked this PR to any issues that it closes.

@lext88
Copy link
Contributor Author

lext88 commented Sep 2, 2024

closes #4702

@lext88 lext88 marked this pull request as draft September 2, 2024 23:57
@lext88 lext88 marked this pull request as ready for review September 3, 2024 23:37
Copy link
Contributor

@TheGamingMousse TheGamingMousse left a comment

Choose a reason for hiding this comment

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

just some style comments - i don't know the specific solution so won't comment on that

content/5_Plat/PIE.problems.json Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
@SansPapyrus683
Copy link
Contributor

please split the solutions into multiple prs

Copy link
Member

@ryanchou-dev ryanchou-dev left a comment

Choose a reason for hiding this comment

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

please create separate prs before applying changes 🙏

thank you for your contribution!

solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2417.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
solutions/platinum/cses-2429.mdx Outdated Show resolved Hide resolved
@TheGamingMousse
Copy link
Contributor

@lext88 are you planning on splitting this PR up? i think it would probably be easier to review that way

@SansPapyrus683 SansPapyrus683 changed the title Update PIE.problems.json Counting Coprimes Solution Sep 15, 2024
int main() {
ll ans = 0;
int N;
vector<int> dp(MAX_X, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is dp initialized here?

also btw, vector autofills to 0.

for (int i = 2; i < MAX_X; i++) {
if (is_prime[i]) { primes.push_back(i); }
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be its own function?

title: Counting Coprime Pairs
author: Alexis Tao
---

Copy link
Member

Choose a reason for hiding this comment

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

could you add an explanation?

const int MAX_X = 1e6 + 1;

int main() {
ll ans = 0;
Copy link
Member

Choose a reason for hiding this comment

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

similar to justin's comment, initialize variables when you use them

// Lambda to initialize the sieve and populate the primes vector
auto init = [&]() {
fill(is_prime.begin() + 2, is_prime.end(),
true); // Mark all numbers as prime initially (except 0 and 1)
Copy link
Member

Choose a reason for hiding this comment

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

formatting looks kind of weird here, why don't you just have the comment on a separate line?

Copy link
Member

Choose a reason for hiding this comment

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

this line is also unnecessary? you only call the function once, just set is_prime[0]=is_prime[1]=false

Comment on lines +93 to +94

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return 0;

};

init(); // Initialize the sieve and primes
cin >> N;
Copy link
Member

Choose a reason for hiding this comment

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

yea move this cin up or move the n declaration down

initialize variables when you use them

Copy link

stale bot commented Sep 29, 2024

This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution!

@stale stale bot added the stale label Sep 29, 2024
Copy link

stale bot commented Nov 5, 2024

Changes requested have not been made. Free free to create a new PR.

@stale stale bot closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants