Restructure coordinate-benchmarks by cdeil · Pull Request #42 · astropy/coordinates-benchmark

cdeil · 2015-01-14T22:41:53Z

This is a complete re-structure (almost a re-write) of the coordinates-benchmarks.
It should not change the results from the existing benchmarks ... it's just a code re-organisation.

Motivation / changes:

Put all code into a package (so that it's separated from output files and we can have utils imported in all scripts)
Use Astropy table and coordinates more instead of ad-hoc solutions (e.g. I/O and handing things around)
Have one command line tool that runs sub-commands (via click)
Add benchmark_horizontal command to run the AltAz to celestial transformations benchmark
Add clean, preview, deploy commands.
Remove astrolib ... see discussion in astrolib celestial results are "random" #44 .

I think this way it will be easier to grow this in the future (e.g. add more precision benchmarks or performance benchmarks)

Comments welcome!

astrofrog · 2015-01-15T11:09:19Z

This looks good so far! +1 to making it into a package.

I have no issues with the click dependency since this is not in the core package.

Small note: for the astrolib files you deleted and added instead of git mv hence the large diff.

cdeil · 2015-01-15T18:06:37Z

I'm running out of time and want to finish this up ... the splitting of the summary table computation and html page generation will have to be done later ... there's already an open issue for that: #35 .

cdeil · 2015-01-15T19:21:43Z

This is 90% done, but I'm out of time for today.
I can finish this up tomorrow morning.

There must be some minor bug because the summary.txt changed where it shouldn't have.

@astrofrog - If you have time to review this, please do.

cdeil · 2015-01-18T12:48:12Z

@astrofrog This is mostly ready ... there's one major issue left which is that the summary now shows larger disagreements that before, although the individual results txt files are identical. So probably I introduced a bug into the code that computes the summary, but I couldn't find it now and unfortunately won't have time to debug this further before Wednesday ...

Current results on the web (which I confirmed locally match what current master dd6c851 gives)

With this PR:

cdeil · 2015-01-21T11:03:18Z

This is the cause of the big changes visible in the summary ... this way to compute separations on the sky in arcsecs gives different results from the _vicenty_dist_arcsec function we used previously.
I'll think the results from this version are incorrect ... not sure where the precision is lost yet ... I'll fix it this afternoon.

Is there a reason we can't just use the one from astropy directly?

@astrofrog - That's what I was doing in this PR ... change from a copy of _vicenty_dist_arcsec in coordinates-benchmark to call astropy.coordinates.angle_utilities.angular_separation which is what is used internally in Astropy.

I made a stupid mistake here ... sep is in radian, not degree ... will fix this now.

cdeil · 2015-01-21T12:46:30Z

As far as I see this PR now gives identical results as before, as it should:

@astrofrog Can you please give this a final review?
I'd like to merge this soon so that work on AltAz benchmarks can start (in future PRs, not here, the diff is too large).

astrofrog · 2015-01-21T13:31:56Z

@cdeil - this looks great to me! Two comments that we can address later (because they are beyond the scope of reorganizing):

We should include units in the tables, maybe we can do this once astropy supports ECSV
We could use jinja2 templates for the HTML output

But this is for the future. For now, please go ahead and merge.

Restructure coordinate-benchmarks

cdeil · 2015-01-21T13:43:37Z

There is already an issue that we should use jinja2: #35
I've made a new issue that we should store the units in the tables: #45

cdeil added 13 commits January 14, 2015 17:08

Update benchmark results for my machine

5747057

Move run_benchmark.py to coordinates_benchmark package

0875691

Add top-level make.py command line tool

2b77657

Remove speed.txt from git repo for now

d3f2991

Update results output (minor changes)

e634f78

Move input generate scripts to package

6c9635c

Restructure coordinates-benchmarks

3f91d23

Move TOOLS and CELESTIAL_SYSTEMS to config

2ca35bb

Move Python scripts to package

8856378

Move horizontal transform benchmarks to package

8ba2806

Move tools folder to output

b0b9b8b

Update README and docs

81baf2b

Add tool info and version check

17c05e9

cdeil added 11 commits January 15, 2015 15:04

Update docs/Specification

d29ebd5

Continue re-factor and implement benchmark_horizontal

f8be54d

Update horizontal output

f6e80e6

Use Astropy table to generate skycoords.txt

3a64d85

Misc updates and change output format precision

f8f6d5e

Update output format (add column names)

00a9cfc

Use tables everywhere; merge utils and config

10470b2

Update astrolib celestial results

d3bc670

Simplify running of celestial benchmarks

276007c

Split html and plot into separate files

bed41b3

Rename module

0781a02

cdeil changed the title ~~WIP: Restructure coordinate-benchmarks~~ Restructure coordinate-benchmarks Jan 15, 2015

Fix plot command

105ac4c

cdeil added 2 commits January 15, 2015 19:17

Re-structure summary output

e514e4d

Finish summary sub-command implementation

fa19f7f

cdeil added 2 commits January 15, 2015 20:16

Update astrolib output results

eb8089b

Update summary output (but there's still a bug)

bb29414

Fix travis-ci build

3f4a8e7

cdeil force-pushed the restructure branch from d5bd226 to 3f4a8e7 Compare January 16, 2015 09:42

cdeil mentioned this pull request Jan 18, 2015

Make coordinates-benchmark work with Python 3 #43

Closed

cdeil added 6 commits January 18, 2015 12:43

Disable Python 3 test on travis for now

15c03fa

Move IDL script and results to a temp folder

d588db1

Remove old pyast/coords_fk5_to_horizontal.txt

2453dc5

Make sure output folders are created

c401ac2

update astrolib results

6c111da

update summary

bfb88a9

This was referenced Jan 18, 2015

Improve AltAz to RaDec accuracy tests astropy/astropy#3296

Merged

astrolib celestial results are "random" #44

Closed

cdeil added 5 commits January 21, 2015 10:33

Remove astrolib from coordinates-benchmark

0de1765

Update summary

b0e274b

Add clean and preview commands

5c855a3

Add back style.css and mechanism to handle static files

7d72bc8

Implement temp workaround for Table bug

7382951

cdeil reviewed Jan 21, 2015
View reviewed changes

cdeil added 2 commits January 21, 2015 13:41

fix bug in angular separation computation

4bc250c

update output

c7b269e

cdeil added a commit that referenced this pull request Jan 21, 2015

Merge pull request #42 from cdeil/restructure

c110a2b

Restructure coordinate-benchmarks

cdeil merged commit c110a2b into astropy:master Jan 21, 2015

cdeil mentioned this pull request Jan 21, 2015

Change from degrees to radians #45

Open

cdeil mentioned this pull request Jan 21, 2015

Update IDL files #36

Closed

Navigation Menu

Repository navigation

Restructure coordinate-benchmarks - #42

Restructure coordinate-benchmarks#42
cdeil merged 43 commits into
astropy:masterfrom
cdeil:restructure

cdeil commented Jan 14, 2015

Uh oh!

astrofrog commented Jan 15, 2015

Uh oh!

cdeil commented Jan 15, 2015

Uh oh!

cdeil commented Jan 15, 2015

Uh oh!

cdeil commented Jan 18, 2015

Uh oh!

cdeil Jan 21, 2015

Uh oh!

astrofrog Jan 21, 2015

Uh oh!

cdeil Jan 21, 2015

Uh oh!

cdeil Jan 21, 2015

Uh oh!

cdeil commented Jan 21, 2015

Uh oh!

astrofrog commented Jan 21, 2015

Uh oh!

cdeil commented Jan 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cdeil commented Jan 14, 2015

Uh oh!

astrofrog commented Jan 15, 2015

Uh oh!

cdeil commented Jan 15, 2015

Uh oh!

cdeil commented Jan 15, 2015

Uh oh!

cdeil commented Jan 18, 2015

Uh oh!

cdeil Jan 21, 2015

Choose a reason for hiding this comment

Uh oh!

astrofrog Jan 21, 2015

Choose a reason for hiding this comment

Uh oh!

cdeil Jan 21, 2015

Choose a reason for hiding this comment

Uh oh!

cdeil Jan 21, 2015

Choose a reason for hiding this comment

Uh oh!

cdeil commented Jan 21, 2015

Uh oh!

astrofrog commented Jan 21, 2015

Uh oh!

cdeil commented Jan 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants