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

implementation notes doc needs fleshing out #308

Closed
jbphet opened this issue Dec 15, 2020 · 5 comments
Closed

implementation notes doc needs fleshing out #308

jbphet opened this issue Dec 15, 2020 · 5 comments

Comments

@jbphet
Copy link
Contributor

jbphet commented Dec 15, 2020

From the code review in #286, specifically the item "Does implementation-notes.md adequately describe the implementation, with an overview that will be useful to future maintainers?".

The document as currently written is quite brief and jumps right into some very specific information without describing the overall architecture and design approach. It also doesn't describe any of the key classes or the relationships between them. I think pulling the information from model.md into this document would help (see #307), and providing a good "forest for the trees" sort of explanation as well.

@jbphet jbphet mentioned this issue Dec 15, 2020
@zepumph zepumph self-assigned this Dec 16, 2020
@jbphet
Copy link
Contributor Author

jbphet commented Dec 22, 2020

As I review the code, I'm seeing the terms "antecedent" and "consequent" used a lot. It would be good to mention these as important terms in the implementation-notes document and say a bit about how they are used in this context, or perhaps provide a good link. I vaguely recall the terms, but am in the process of searching online to refresh my memory about them, so some good docs might save time for future devs.

@zepumph
Copy link
Member

zepumph commented Dec 22, 2020

Yes, I had an incorrect understanding of the separation of model.md and implementation_notes.md, as so those are defined briefly in model.md, Most of model.md will end up moving to implementation notes.

@zepumph
Copy link
Member

zepumph commented Jan 26, 2021

I basically merged all of model.md into the implementation notes. @jbphet please let me know if you recommend anything else here, and feel free to close.

@zepumph zepumph assigned jbphet and unassigned zepumph Jan 26, 2021
jbphet added a commit that referenced this issue Feb 1, 2021
@jbphet
Copy link
Contributor Author

jbphet commented Feb 1, 2021

Looks good, this is a big improvement. I have a few comments:

  • First, I made some edits for some minor things that I was fairly certain you'd want to change, please review the commit and feel free to back out anything that was intentional
  • I would suggest a short overview in the model section that describes a few of the relationships between the major players. For instance, I wasn't sure after reading through the RAPRatio.js section if there was just one instance of this per screen or what.
  • I was a bit confused by the opening paragraph of the "Model States" section. I think the states are mutually exclusive, but this paragraph uses singulars and plurals in a way that left me a bit confused. You may want to revise.
  • In the "In Proportion" subsection, there is a sentence that says, "This helps with strange behavior that can be gathered for small ratios". I understood what was meant (at least I think I did), but was wondering if you meant "generated" or something other than "gathered".
  • In the "Far from proportion" section it says, "...there is often less feedback from the sim". The use of the word "often" makes me think that maybe some randomness is involved, but the rest of the section (and my own familiarity with the sim) lead me to believe that there isn't. Can the word "often" be dropped?

@jbphet jbphet assigned zepumph and unassigned jbphet Feb 1, 2021
zepumph added a commit that referenced this issue Feb 1, 2021
@zepumph
Copy link
Member

zepumph commented Feb 1, 2021

Thank you for your suggestions. That was very helpful! Closing

@zepumph zepumph closed this as completed Feb 1, 2021
zepumph pushed a commit that referenced this issue Feb 1, 2021
zepumph added a commit that referenced this issue Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants