Skip to content

Monte Carlo Scratch Implementation #619

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

Merged
merged 10 commits into from
Apr 28, 2020

Conversation

dovisutu
Copy link
Contributor

@dovisutu dovisutu commented Jul 3, 2019

No description provided.

@c252 c252 self-assigned this Jul 4, 2019
Copy link
Member

@leios leios left a comment

Choose a reason for hiding this comment

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

I'll let @c252 handle the actual review, just wanted to throw my 2 cents in. I'm fine with the code as-is.

@leios
Copy link
Member

leios commented Jul 22, 2019

@c252 You mentioned you wanted to tackle this one; however, the only inconsistency I see is that there is no radius parameter. And this is inconsistent for monte carlo in general (#625).

What are your thoughts?

@c252
Copy link
Member

c252 commented Jul 22, 2019

If it is alright with you then I am fine with it.

@dovisutu
Copy link
Contributor Author

Maybe I can add radius in, along with a note // the radius is 1.

@leios
Copy link
Member

leios commented Jul 23, 2019

Sure, that would work for now at least... And we have the scratchblocks code, so it's no where near as hard to modify as before.

@dovisutu
Copy link
Contributor Author

dovisutu commented Jul 23, 2019

BTW, after I looked up the source code of scratch3, I realized the random generator can generate doubles as long as at least one of FROM and TO is double.
I'll change it too s.t. it's less confusing.

@leios
Copy link
Member

leios commented Jul 23, 2019

Oh, nice! So we don't need the arbitrarily large value anymore!

Copy link
Member

@leios leios left a comment

Choose a reason for hiding this comment

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

One minor change. Thanks for the work on this PR!

 - Add `radius` parameter
 - Directly generate doubles
@dovisutu dovisutu force-pushed the monte_carlo_in_scratch branch from 14f8041 to 3e66435 Compare July 25, 2019 02:31
@leios
Copy link
Member

leios commented Jul 26, 2019

There seems to be a lot of empty space under the Algorithm.svg image. Could this be cropped?

@c252
Copy link
Member

c252 commented Jul 26, 2019

I think somewhere in the program you might need to set the radius variable to 1 because in scratch3 the variables will default to 0.

@dovisutu
Copy link
Contributor Author

About radius, I globally ran a block (set radius to 1) and never change it afterwards.
I don't add it in algorithm, because I add a comment // the radius is 1. If add initialization, the comment can be removed.

@c252
Copy link
Member

c252 commented Jul 26, 2019

I figured that was how you did it, but my worry was that if someone else copied this scratch code and ran it the radius would be 0 and it wouldn't work properly. (It's not a very big deal)

@dovisutu
Copy link
Contributor Author

About Algorithm.svg, It's because I go back and use the tool here instead of this one because of better quality at comments, but it seemly like has a wrong size.
The blank can be cropped simply by changing the width and height approximately.

@dovisutu
Copy link
Contributor Author

dovisutu commented Jul 26, 2019

What I thought is if they see the comment, they should know they should set radius to 1.
Although it's possible that somebody who know nothing about scratch just mechanically copied it...?

 - Add main loop
 - Fit `width` into .md
@c252
Copy link
Member

c252 commented Aug 13, 2019

This looks good to Me!

@leios
Copy link
Member

leios commented Aug 16, 2019

The Algorithm.svg file does not seem to be viewable on my end anymore.

@dovisutu
Copy link
Contributor Author

Can't view again? Umm... I'll fix it if I have time...
(possibly not because schoolwork)

@berquist berquist added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Sep 1, 2019
@leios
Copy link
Member

leios commented Apr 27, 2020

This PR is ready to go, except that the svg image has whitespace under it when rendering the Algorithm Archive. I can try to take a look at it in more detail if you cannot figure this out. There are some issues with css + svg.

We can relatively easily convert everything to png and it will work fine; however, there might be a way to get this to work with a viewbox

This was tested in Firefox and chromium on linux.

@dovisutu
Copy link
Contributor Author

Yeah, that is confirmed. Setting transform: scale(1) fixes it, as what I did in #682.
Now this should be fixed - tested on (old) Edge and Chrome.

Copy link
Member

@leios leios left a comment

Choose a reason for hiding this comment

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

Great! Thanks for fixing the cropping issue!

@leios leios merged commit 3be0e92 into algorithm-archivists:master Apr 28, 2020
@dovisutu dovisutu deleted the monte_carlo_in_scratch branch May 30, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants