search

sympy / sympy_benchmarks

Some benchmarks of SymPy
14 stars 32 forks source link

Updated readme #57

Closed ShubhamKJha closed 5 years ago

ShubhamKJha commented 5 years ago

Updated README and added instructions to let users benchmark their local forks. Also, changed the README to markdown. The routine ./run_benchmarks.sh cannot be used in Windows environment. I would like to write such routines in Python to make them platform independent.

bjodah commented 5 years ago

Could we start with a textual update of README.rst (i.e. not chainging to markdown and changing the text in the same PR since it makes it harder to review). Also, there's already asv.conf.venv.json (or something) similar in the repo root (README should be updated with correct name of this file. Do we need another config file?

Creating cross-platform invocation scripts which work on windows sounds fine. Maybe a separate PR for that as well?

ShubhamKJha commented 5 years ago

Could we start with a textual update of README.rst

Is it possible to compare the README.rst from the master with README.md from my branch?

Also, there's already asv.conf.venv.json

I tried to remove the use of .\run_benchmarks.sh file from the README ( which is meant for common users I think). So, I have added the config file asv.conf.json which is called by asv by default. The other asv.conf.venv.json I think can be removed.

bjodah commented 5 years ago

Could we start with a textual update of README.rst

Is it possible to compare the README.rst from the master with README.md from my branch?

A new PR will be the simplest.

Also, there's already asv.conf.venv.json

I tried to remove the use of .\run_benchmarks.sh file from the README ( which is meant for common users I think). So, I have added the config file asv.conf.json which is called by asv by default. The other asv.conf.venv.json I think can be removed.

I disagree, the unconventional naming is done on purpose, some use conda, others virtualenv.

ShubhamKJha commented 5 years ago

A new PR will be the simplest.

OK

I disagree, the unconventional naming is made on purpose, some use conda, others virtualenv.

What I tried to do here is that to allow the default to be virtualenv and letting users who use conda to use asv.conf.conda.json instead( also gave instructions to change the default behavior). I left asv.conf.venv.json for cross-platforms invocation routines. But an asv.conf.json should be there, it allows asv to be used without --config args.

sylee957 commented 5 years ago

Markdown and rst have different syntax, so the diff may work not neatly.

I wonder if you had used an editor that automatically converts rst to markdown, and if you had used it, you may have to find a way to convert it back

sylee957 commented 5 years ago

And it looks like conda users can experience some unexpected behavior when missing the —config flag, only to make some slight advantage for venv users.

ShubhamKJha commented 5 years ago

And it looks like conda users can experience some unexpected behavior when missing the —config flag, only to make some slight advantage for venv users.

conda is not added to path when anaconda is installed(atleast not in Windows), maybe that is the reason.

asmeurer commented 5 years ago

It looks good to me. I would just review the README as-is, without worrying about what was there before. I left a few comments and suggestions.

I wish asv "just worked" for asv compare COMMIT1 COMMIT2, so that you didn't have to manually do the run command for each one.

ShubhamKJha commented 5 years ago

I wish asv "just worked" for asv compare COMMIT1 COMMIT2, so that you didn't have to manually do the run command for each one.

I will write a Python routine to automate this.

moorepants commented 5 years ago

I'd prefer the readme to stay as rst and to be able to view the changes to that document.