transientskp / tkp

A transients-discovery pipeline for astronomical image-based surveys
http://docs.transientskp.org/
BSD 2-Clause "Simplified" License
19 stars 15 forks source link

Store varmetric values in a table #488

Closed timstaley closed 8 years ago

timstaley commented 8 years ago

Rebased version of https://github.com/transientskp/tkp/pull/469 (Actually the rebase was non-trivial, which is why I didn't force-push over the old branch. Think I got it, though.)

Hey @gijzelaerr , took a look at this, looks good (I think! It's taken me all day just to get back up to speed, so I've not dived deep into the queries).

Two concrete suggestions:

And some comments:

Probably some of this should go in an issue... tomorrow.

gijzelaerr commented 8 years ago

1> we discussed this in oxford, and agreed that for now the name 'sqlalchemy' would be best, and we split it up later when the code gets more specialised.

2> yes, the only thing missing was the doc. Indeed I wanted to switch to docstrings, but that is just too much work for now.

3> https://github.com/transientskp/banana/pull/101

4> Yes, the whole wrapper stuff is a bit clunky. Wanted to replace that one day when we would refactor the whole main loop.

5>I couldn't really understand what was happening, started to replicate the behaviour in SQLAlchemy to get myself familiar with the technology and it grew from there. Probably yours is more advanced.

6> I'm worried also we don't test it enough, although this is already more tested than the augmented runningcatalog view. I wasn't really sure what would be a good set of tests, and there was nobody around to discuss this with :'(

gijzelaerr commented 8 years ago

test this please

gijzelaerr commented 8 years ago

@timstaley what do you think is the status of this now?