-
Notifications
You must be signed in to change notification settings - Fork 587
Mojo Solution Contribution #1003
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
Mojo Solution Contribution #1003
Conversation
Mojo Solution for contribution
adjusted formatting strings for the tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your submission!
I have two minor (textual) remarks, the second of which you could even ignore - I acknowledge that is a very nit-picky one indeed.
Other than that, it looks good to me!
As per feedback, updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As this is a new solution in a new language, I will also ask for a review from @marghidanu.
If possible, pause on reviewing or merging this with the main branch. I reviewed it over my lunch break and it's technically not faithful. As the arrays are metaprogramming, not runtime dynamic I made this when the kids got to bed, I was tired and my dyslexia kicked in. |
Of course we can pause the review and merge process at any time; as far as I know all maintainers here are still human. :) In any case, I really appreciate your transparency and proactiveness on this. I'll just wait for your next update. |
Faithful implementation and cleaned up messy print method.
I updated it now, thank you for the understanding. I made some quality of life changes and showed a few different ways of approaching the solution, both through metaprogramming and dynamic allocation (faithful). Hopefully this shows off the language. |
Corrected formatting and added clarity to the 8bit explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed your solution again after your recent updates, and I found some small things that I missed before, that need to be changed for your solution to be successfully included in the automated benchmark and the reporting thereof.
PrimeMojo/solution_1/primesieve.mojo
Outdated
fn printResults(self: Self, duration: UInt64, passes: UInt64) -> None: | ||
try: | ||
var final_string = ( | ||
"ELucasCurrie_1bit;{0};{1};1;algorithm=base,faithful=yes,bit=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit=1 should be bits=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be named Dockerfile (lower-case "f")
PrimeMojo/solution_1/primesieve.mojo
Outdated
fn printResults(self: Self, duration: UInt64, passes: UInt64) -> None: | ||
try: | ||
var final_string = ( | ||
"ELucasCurrie_8bit;{0};{1};1;algorithm=base,faithful=yes,bit=8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit=8 should be bits=8
PrimeMojo/solution_1/primesieve.mojo
Outdated
fn printResults(self: Self, duration: UInt64, passes: UInt64) -> None: | ||
try: | ||
var final_string = ( | ||
"ELucasCurrie_1bit;{0};{1};1;algorithm=base,faithful=no,bit=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit=1 should be bits=1
PrimeMojo/solution_1/primesieve.mojo
Outdated
fn printResults(self: Self, duration: UInt64, passes: UInt64) -> None: | ||
try: | ||
var final_string = ( | ||
"ELucasCurrie_8bit;{0};{1};1;algorithm=base,faithful=no,bit=8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit=8 should be bits=8
Updated strings to print bits rather than bit
Updated the case for the docker file
Modified the readme to reflect the changes made.
Thank you, for looking at it again and I made the changes so it should be successful for the automation now. Also apologies for making you have to look that close into it. |
No worries. I actually took a closer look because I found I overlooked some similar things in #998, quite recently. Thanks for following up so quickly! |
There is actually one more thing: the meta and non-meta implementations use the same labels in their output lines, which confuses the benchmark reporting. You could add "_meta" to the meta implementation variations' output labels to make the distinction between the two approaches apparent in the benchmark results. |
Updated identifier & Readme
Yeah no problem, and I modified it to now correctly show the identifiers. I might have factored it out by accident when making the interface. Hopefully, all is well now :) but feel free to reach out again if there are any issues, I'll address them as soon as I can. Thank you for being so understanding! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, again.
Mojo bool is actually a structure, thus not a true 8 bit. Mojo also does bounds checking by default when indexing into an array. Modified to unsafe get.
Sorry, but I have to add one primary change specifically here in the documentation: Mojo BitSet Docs It mentions bools use 64x more memory which made me look into it and they are structures even though it says primitive scalar values. Meaning they use the full address space, this is a kind of language gotcha I didn't know about. However, I updated it to UInt8 and made a small optimization for indexing since I had to make this commit. |
I was waiting for this for some time, glad to see a solution in Mojo! LGTM |
@Evan-LucasCurrie I'm just asking because you've been updating this solution over the past few days: are we good to merge, as far as you're concerned? |
@rbergen Yeah, it's in a good place to merge. There is for sure performance on the table but that's always the case. I'm sure either myself or someone in the mojo community will eventually improve upon it. Much thanks, honestly you all made this process super pleasant and I appreciate it! |
Alright, that's great! If CI passes I'll merge, then. And thank you for the kind words! |
Description
This PR contains two mojo solutions:
Contributing requirements
drag-race
as the target branch.