spacetelescope / exovetter

Exoplanet vetting
https://exovetter.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Add LPP code and vetters.py. sm add lpp #19

Closed mustaric closed 4 years ago

mustaric commented 4 years ago

I've added the LPP Code. Let me know if this is ready to merge.

Close #9, close #11 , fix #12

EDIT: Acceptance criteria

mustaric commented 4 years ago

So I can't get the tests to pass without adding a TCE class. It seems that should be a separate pull request because it is a somewhat larger and separate task...yes?no? To do that I need to merge this branch so I can start developing that code on a new branch with this code available. This is where I really don't understand the git workflow. In the mean time I'll work on the small stuff.

mustaric commented 4 years ago

Ah I see, you looked at main.py. I've stopped using that file and it is out of date. I should have removed the duplicate code and moved the file. ok. I'll make that change.

pllim commented 4 years ago

For a test that you know will fail until more things are fixed in the future, you can try to use https://docs.pytest.org/en/latest/skipping.html#xfail-mark-test-functions-as-expected-to-fail and provide a reason. Thanks!

pllim commented 4 years ago

I pushed this as far as I could, but I don't know how to fix 'Lppdata' object has no attribute 'period', which I think is a real bug. Here is a summary of what I did in my commit:

I also added acceptance criteria above. Only merge when all boxes are checked.

mustaric commented 4 years ago

It looks like you were using the wrong version of the map file. I fixed that, then uncommented all the parameters that are required to run lpp that are indeed in this version of the map file. I then fixed the test to have more tolerance. It is again passing for me. It doesn't look like the run_one_lpp test is passing above, but it is passing on my machine, so I'm a bit at a loss.

pllim commented 4 years ago

E FileNotFoundError: [Errno 2] No such file or directory: '/Users/smullally/Python_Code/exoplanetvetting/original/dave/lpp/newlpp/map/combMapDR25AugustMapDV_6574.mat'

You need to put the file online somewhere. If it is less than 5 MB, I would say just put it in this repo at this point to get things moving forward.

pllim commented 4 years ago

Also you reintroduced some PEP 8 warnings. FYI.

pllim commented 4 years ago

If you want to put the data file in this package, do the following:

  1. Put it in a new exovetter/data/ directory.
  2. Add this in setup.cfg:
[options.package_data]
exovetter = data/*
  1. To use it in your code (you need to reinstall your local package unless you installed it in editable mode already):
import os

from astropy.utils.data import get_pkg_data_filename

def myfunction(...):
    filename = get_pkg_data_filename(os.path.join('data', 'datafile.mat'))
  1. If things work, add the data file and new code into version control.

Please let me know if you need help. Good luck.

mustaric commented 4 years ago

Looks like I had accidentally added one of my local testing files to the github repo...which was using the local copy of the map file. So I removed that and it looks like tests are passing. Next step is to find a way to actually fix the pep8 violations without doing it by hand.