search

phetsims / shred

A library of JavaScript code that is used in PhET simulations that depict atoms, subatomic particles, and atomic structure. This was originally created to contain the code that is shared between the "Build an Atom" and "Isotopes and Atomic Mass" simulations, thought it may be applied to additional simulations in the future.
GNU General Public License v3.0
1 stars 6 forks source link

Generalize ParticleAtom #18

Closed jessegreenberg closed 8 years ago

jessegreenberg commented 8 years ago

For use in rutherford-scattering, it would be helpful if the nucleon radius could be added to options. Nucleons in rutherford-scattering are much smaller than isotopes-and-atomic-mass and build-an-atom.

Also, for multiple nucleons (more than 5) the radius of a single z layer of particles is calculated by placementRadius += nucleonRadius * 1.35 / level;

The nucleus in rutherford-scattering can have up to 250 nucleus, and scaling by 1.35 makes the nucleus look quite dense. It would be helpful if 1.35 could change. Maybe it could be a function of the number of nucleons and the width of an single particle.

From https://github.com/phetsims/rutherford-scattering/issues/59

jessegreenberg commented 8 years ago

In the commit above, the nucleon radius is now optional in ParticleAtom. @andrewadare would you mind reviewing these changes since you recently gave shred a full review?

Pinging @jbphet in case he would like to take a look as well.

andrewadare commented 8 years ago

@jessegreenberg Great idea to make the radius configurable. You've removed the 1.35 as a "magic number" from the radius scaling, which is a plus. (Couldn't resist the play on words there).

On the other hand, looks like you've introduced a new set of magic numbers in the linear mapping here. You followed a good practice by noting when hard-coded parameters were eyeballed, but it would be even better to assign to meaningfully-named vars the input/domain boundaries (the first two args) and the output/range boundaries (the second two args).

Also, it would help to add a bit more to the comment explaining exactly what you're visually trying to achieve with this.

It's especially valuable to do so because the independent variable range is reversed (wouldn't hurt to explain that). Typically, wouldn't people reverse the range of the output variable to make it clearer that the mapping has a negative slope?

jessegreenberg commented 8 years ago

All great points @andrewadare, thanks for the review! Commit above should enhance clarity of the linear map with factoring and additional documentation. Assigning back to @andrewadare for further discussion if necessary.

andrewadare commented 8 years ago

LGTM, all my points have been addressed.

jessegreenberg commented 8 years ago

Great, thanks for reviewing @andrewadare! Closing.