the complete fft inv fft and frequency finder

[zbpetersbuf]

• my code does lint

-has the fft for the power, fft for the ins fft, inv fft, and the frequency finding equation

[zbpetersbuf]

ok so in my unit tests in each test I did have data and not time_series_data, but then pylint said this was bad so I changed it and then It liked it idk what to do if pylint made me change something that pytest doesn't understand

[avgagliardo]

I'll volunteer to review this code if it hasn't been assigned yet. (if its ready)

[SchrodingersStruggle]

@avgagliardo
Added you as a reviewer!

[laserlab]

@avgagliardo If you could give @zbpetersbuf some help with a fixture it would be great, since you are one of the only ones that did it so far.

[avgagliardo]

sure thing, is there a specific format or fields that you want @zbpetersbuf?

For example, here is one measurement (in json) from a set of discrete measurements

{
        "site_code": "AVI",
        "year": "1983",
        "month": "5",
        "day": "12",
        "hour": "20",
        "minute": "0",
        "second": "0",
        "datetime": "1983-05-12T20:00:00Z",
        "time_decimal": "1983.3611872146118",
        "air_sample_container_id": "814-81",
        "value": "1637.071",
        "value_unc": "3.3",
        "latitude": "17.75",
        "longitude": "-64.75",
        "altitude": "6.0",
        "elevation": "3.0",
        "intake_height": "3.0",
        "method": "P",
        "event_number": "10804",
        "instrument": "C2",
        "analysis_datetime": "1983-05-17T00:12:00",
        "qcflag": "..."
    },

[avgagliardo]

@zbpetersbuf also let me know if you want me to review the code yet. I'll do it tonight if I don't hear anything but I don't mind doing it again later if its not ready.

[zbpetersbuf]

ya you can review it, it should be done I hope

[zbpetersbuf]

ok for the data you only need datetime and the value we are concerned with, so that might be co2 concentration

[avgagliardo]

ok for the data you only need datetime and the value we are concerned with, so that might be co2 concentration

Okay, gotcha. Check out PR #36 if you still need fixture data. Starting the review now too!

[avgagliardo]

Code is well-formed and lints at 10/10.

Docstrings are present, though module docstrings are a little terse.

Unable to run the tests because the time series data needs to be defined, but I can pull down the data and rerun the tests whenever its updated.

For now I will mark as requesting changes, for the sake of getting the unit tests up and running.

I'll watch for notifications but feel free to ping me when you push and I'll review it again ASAP.

[avgagliardo]

docstring should maybe include a small bit about what tests are included or what functionalities are being tested for posterity.

[avgagliardo]

code is well-formed, and lints at 10/10

Unit-tests do not currently run because the time_series_data object does not seem to be defined. If you still need fixture data let me know, but there is a bunch in PR #36 you can use or I could prep some for you if it needs to be customized.

[zbpetersbuf]

its cause pylon told me to change the name of the input data for each set function, it gives me a worse grade when I do it correctly idk what to do for this I can change it back in a bit

[avgagliardo]

So I mentioned a bit of a hacky way to get around this in class today:

Lets say the fixture is some dict:

data = {
    "datetime":"2014-01-02T05:00:00Z",
    "value":1936.48
}

and pylint is complaining because this object has been defined separately in two different places (which, in this case is actually what we want) then a very easy way (though I'll admit that this is not the best practice) is to modify the fixture like so:

data = {
    "datetime": "2014-01-02T05:00:00Z",
    "value": None
}

data['value'] = 1936.48

If you modify the initial definition of the object, the linter won't recognize the duplicate definition. In this case, I just set the initial "value" to None and then updated it to the proper value immediately afterwards.

[SchrodingersStruggle]

@zbpetersbuf
Just so you're aware, in the main repository I moved the python files out of the folder and into main, to make sure people can see them easier to edit.

I did this because people were having a hard time with working on the same file, some would create new ones, so this hopefully will encourage people to work on the same files.

With this in mind, you will have to make small changes to make sure conflicts are resolved.

[zbpetersbuf]

@SchrodingersStruggle could you clarify what changes you made that you want me to resolve?

[SchrodingersStruggle]

@SchrodingersStruggle could you clarify what changes you made that you want me to resolve?

All I did was take the python files out of the fit_folder so people can see them upon opening the repository and encourage working in the same module.

All you need to do is be sure the changes you're making would merge to the correct files by taking them out of the fft_folder for your pr.

[zbpetersbuf]

alright ill do that I don't understand why but its all good with me

[zbpetersbuf]

they have been moved

[laserlab]

@avgagliardo @zbpetersbuf Can you solve this fixture issue making py test fail?

fixture 'data' not found

[avgagliardo]

@avgagliardo @zbpetersbuf Can you solve this fixture issue making py test fail?

fixture 'data' not found

I'll take a look at it now.

[avgagliardo]

Has anyone defined the shape of the objects we are testing? I have data that I can form up into fixtures, but I don't see a direct path to using it so we might need to rework the unit tests a little bit.

The big thing is that the unit tests do not use pytest, but assert equivalence of object states manually. That is probably not what we want.

My advice would be to start with something like this:

import pytest

@pytest.fixture
def data():
    trange = pd.date_range(datetime.now(), datetime.now() + timedelta(days=9), freq='d')
    return pd.Series([1, 2, 3, 2, 1, 2, 3, 2, 1, 2], index=trange)

Prepending this into the file before the unit tests will import pytest, and define the existing mock data as a fixture using pytest's decorator syntax. On my testing runs, this makes all the unit tests pass.

If we want to include some of the CO2/CH4 scraped data as a fixture, let me know and we can do that too. I would push a PR to fix it myself, but I have a couple of nightmare deadlines coming up that I need to take care of.

If there are any issues with this though ping me (on github or disco) and I'll be around to help.

[avgagliardo]

Also @zbpetersbuf I will keep watch so I can approve the review, but again feel free to ping me when its ready.

[avgagliardo]

Oh one last thing that I should clarify-- I didn't check that the unit tests return CORRECT results, just that that run properly. So if there is any logic in there that doesn't cause a first-order testing failure (in this case through the assert statements), then that bug would remain.

[laserlab]

Merging this now so we can keep going. The py test fixture has to be taken care of in a separate PR