Skip to content

Conversation

@douglasdennis
Copy link
Contributor

While I was reviewing the code for something else I noticed a dropna call that didn't have inplace set to True and wasn't getting assigned to a new variable. I set inplace to True as it seemed that was what was being done elsewhere in the code. If you prefer the dropna to be removed then I can make that edit instead.

@elbeejay elbeejay added the bug Something isn't working label Nov 22, 2022
@elbeejay elbeejay self-requested a review November 22, 2022 19:12
@elbeejay
Copy link
Contributor

elbeejay commented Nov 22, 2022

Nice catch @douglasdennis and thanks for opening this PR. Would you mind adding a unit test that ensures we get the right behavior moving forward?

Something relatively small like the following should work, you could add it to tests/nwis_test.py. It fails for me using the current master branch, but will pass once your change is implemented.

def test_preformat_peaks_response():
    # make a data frame with a "peak_dt" datetime column
    # it will have some nan and none values
    data = {"peak_dt": ["2000-03-22",
                        "2001-06-08",
                        np.nan,
                        "2002-04-29",
                        "2003-02-23",
                        None,
                        "2003-12-12"],
            "peak_va": [1000,
                        2000,
                        3000,
                        4000,
                        5000,
                        6000,
                        7000]
    }
    # turn data into dataframe
    df = pd.DataFrame(data)
    # run preformat function
    df = preformat_peaks_response(df)
    # assertions
    assert 'datetime' in df.columns
    assert np.isnan(df['datetime']).sum() == 0

@douglasdennis
Copy link
Contributor Author

I'd be happy to add a test. I should be able to get that done tomorrow. Thank you for putting one together for me!

@douglasdennis
Copy link
Contributor Author

@elbeejay Sorry for the delay but I added the unit test. The test failure on the previous run looks to be an issue with setting up the build job and not related to this PR. Let me know if I'm missing something though.

Copy link
Contributor

@elbeejay elbeejay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @douglasdennis! Feel free to reach out in the future if you run into problems using dataretrieval or need help designing queries or manipulating the data.

@elbeejay elbeejay merged commit 44d1edd into DOI-USGS:master Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants