Fix: update variable name to remove reference before assignment error

[Graham-EGI]

Snippet from error (happens a bit after the changed line in PR):

 y = y1[ms]
UnboundLocalError: local variable 'y1' referenced before assignment

[akeeste]

@Graham-EGI thanks for this catch. Confirming that this is a necessary change. @ssolson Since this is a bug, I will merge this into the master branch, then pull the commit into Develop so that it is in both places. Noting that we do test this function, but the bug is within an if statement and the test data apparently does not meet that condition.

[ssolson]

Hey @Graham-EGI we made some fixes to the testing module to fix the failing tests. Could you merge the latest Development branch with this PR and see if it fixes the failing tests for you?

[Graham-EGI]

Hey @ssolson , not sure if there was a miscommunication, but I wasn't having any issues with any tests, just the code that is changed in this pr. When using the wave.contours.samples_contour function in a project leveraging MHKiT quite a bit, some of the data I used for testing the integration with other services in the project met the if condition wrapping the bug that maybe is missed in tests.

Quick look at develop and master both still have the y variable that throws the error. Hopefully I'm not misunderstanding!

[ssolson]

@Graham-EGI I see you guys are trying to update master not Develop. I am referring to the failing CI tests on this PR

@akeeste if you push this to our master branch the tests will fail. Its a bad look to me to have that failing but we can if you think that is best. It will not update pip installations unless you do a new release. I am more in favor of getting the current PRs through and including this in a release once we settle the hindcast issues.

[cmichelenstrofer]

@ssolson it seems the CI tests are failing for reasons other than the modification in this PR (literally one character changed).

[ssolson]

@ssolson it seems the CI tests are failing for reasons other than the modification in this PR (literally one character changed).

I know that, I indicated to @akeeste in my response above that any push to the master would result in a failure. This is due primarily to the netcdf #177 update and the hindcast API #194. If the latest Develop branch was pulled and merged to this the tests would not fail.

[Graham-EGI]

@ssolson Oh sorry guys, I didn't realize the PR's needed to be made to/from the Develop branch. Maybe this PR can just wait to be merged into master until those test fixes from develop are included in a new release? Since it's such a small change, and I'm currently using the fixed version outside of MHKiT in our application, it doesn't need to go through right away.

Sorry for any headache this caused!

[ssolson]

@Graham-EGI no this is great. The problems are entirely outside the scope of this issue and switching PRs between master and Dev is quite simple.

@akeeste do you agree with Gram/ my proposal that we include this bug fix in the next release?

[akeeste]

@ssolson Yes we should certainly include merge PR by the next bug fix release.

Personally I prefer to merge this PR immediately because it fixes a known bug. The tests on master will appear as failing, but really I think the greater point is that the master branch tests are already failing. To my understanding, they only appear as passing because the master branch tests have not run since the last update, July 13. I'd rather have users and our team award that we have issues that need to be resolved.

[ssolson]

That is fine. If you are going to do a minor release why not include everything in the dev branch?

[ssolson]

Superseded by #208.