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

comment fixes, README updates, remove extra sympy imports #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phiat
Copy link

@phiat phiat commented Oct 27, 2020

👍 Thanks for posting this project 👍.
Trying to learn some of the techniques and ended up fixing some minor spelling and comment formatting. Only non-comment change is removing an extra sympy import.

did:

  • capitalize and space out comments (did not capitalize commented code)
    • #this function does x ... -> # This function does x
  • fix minor spelling and grammar
  • fix other minor comment inconsistencies
  • add TODO # TODO: Fix this to work with the other variables constant (S_gen_sym.py)
  • add wiki link for Pareto Frontier to README
  • fix README example verbiage to match example parameters (30 vs 60)
  • remove extra sympy imports (Edit: noticed this is in a few functions. Only removed one instance. still need to remove others)
  • add MIT license badge to README
  • convert some """ multiline comments """ -> # single line
  • add newline to start of multiline comments

did not:

  • remove these lines (maybe you want them) ###############################################
  • change wording of author(s)
  • remove: # In[2]: (get_pareto.py:44)

questions:

  • where is: "ai_feynman_example.py contains an example of running the code on some examples (found in the example_data directory)" ?
  • would you want a little more thorough code reformat / refactor (move hardcoded strings/paths to config)? I am not a python guru, but will give it a go.
  • is this repo meant to be maintained somewhat (are you accepting PRs)? Or should I just fork and run with it? Thanks again

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.

1 participant