mommermi / photometrypipeline

automated photometry pipeline for small to medium-sized observatories
GNU General Public License v3.0
62 stars 26 forks source link

fixed photometry output file not being machine readable #13

Closed boada closed 7 years ago

boada commented 7 years ago

also fixed some pep8 stuff...

Basically, there was a small typo that prevented something like numpy from reading both the header and the data from the photometry_*.dat file that is created. Fixing this makes it easy to take the zeropoint calculated by PP and read it into another pipeline. I fixed this little typo, and I also cleaned up the little section with some pep8 changes. Those pep8 changes don't really do anything except make the code more pythonic, and help relieve some of the stress on my syntax checker. =)

mommermi commented 7 years ago

Hmm. What is causing the trouble? The difference between '# filename... and #filename...? I would like to have the column names aligned with the actual data. We could have the column names right-aligned and preserve the machine-readability and human-readability this way.

Thanks for checking the spaces!

boada commented 7 years ago

The problem was on lines 485-486. There wasn't a space at the end of 485 so it was running into 486.

Personally, I don't think having the columns line up with the header makes much difference. My terminal is only going to be so wide (often about 85 columns) so anything more than wraps around and so it doesn't line up anyway.

There are a bunch of pep8 issues. Like a bunch, in pp_run I see 75. I don't really think pep8 changes alone are enough for a pull request. You've obviously formatted the code in such a way, and making the style changes would mess all that up. We can talk about if you want to bring everything (or most everything) into pep8 compliance. But that is for a different request/issue.

mommermi commented 7 years ago

Got it! Still, I would prefer the headers to be aligned with the data.

I have to admit that I never paid much attention to the details in pep8. In the wiki I put a list of todo things for the future, one of which is a complete overhaul that would implement all the pipeline functions in a coherent class structure. That might be a good time to make everything pep8-compliant. I'll add it to the list.

boada commented 7 years ago

It's your baby.

I started a branch where I did a very minimal job of getting everything packed into something that could be installed with PIP. The problem that I ran into is that some of the files need to import things from itself, like the tasks in _pp_prepare. This is an issue when you aren't calling the files directly as executables, but instead want to put copies of them into a bin folder somewhere.

I can start a pull request on that if you like. But it's certainly not ready for prime-time. It would need to be in a development branch and something we could really pound on.

mommermi commented 7 years ago

It's your baby. Sorry, sometimes I'm kind of oldschool...

PIP sounds like a great idea, I totally support this! However, I am a little bit concerned that we have to publish a new release every time we push something to the repo. Any thoughts on that issue?

mommermi commented 7 years ago

do you agree with the current version?

boada commented 7 years ago

Yeah sure. It's whatever you wanna do. I just wanted the output to be read in by numpy.

PS. You don't need the + at the end of each line. Adjacent strings in Python are automatically concatenated.

mommermi commented 7 years ago

PS. You don't need the + at the end of each line. Adjacent strings in Python are automatically concatenated.

... you never stop learning...

Thanks!