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

[Sieve]: Draft approaches #3626

Merged
merged 23 commits into from
Feb 12, 2024
Merged

[Sieve]: Draft approaches #3626

merged 23 commits into from
Feb 12, 2024

Conversation

colinleach
Copy link
Contributor

Parking this here for now. I'll check it through again tomorrow, when I hope to be more awake.

Unless you get to it first (as a displacement activity?)

I've temporarily put a graph in the Performance article, to help us see what's going on. The intention is that this should be removed before merging.

Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in getting this done! Nice work -- I especially like the performance article and code. 😄

I had the usual nits -- I sorta felt like we had to remove single letter variable names, and add in a few things. But feel free to ignore these if they irritate you.

Remind me before we merge to remove the graphs and either reproduce them for the images directory .... or not. 😄 Wonder if we should provide more explanation for the code that does the slope graphing? Something to ponder a bit (but we have time on this one so.)

colinleach and others added 13 commits February 11, 2024 15:00
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Does this add a spurious extra space after the link?

Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
To save us forgetting it later.
I didn't intend to commit this in the first place.
@colinleach
Copy link
Contributor Author

Wonder if we should provide more explanation for the code that does the slope graphing?

I'm open to suggestions, but this risks getting pretty deep into pandas and numpy.linalg.

Somehow, the closing of the codeblocks got dropped.  Added them back in, along with final typo corrections.
@BethanyG
Copy link
Member

Ugh. Somehow, the backticks went missing on a lot of the code blocks, and then adding them back in created issues in the web UI. Finally had to pull it down and edit on my desktop. Hopefully, things all work now. By the looks of the preview and links, it's been fixed. I am going to merge this, and we can do a quick PR in the morning if there are lingering issues.

@BethanyG BethanyG merged commit 7e3a633 into exercism:main Feb 12, 2024
8 checks passed
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.

2 participants