NOC-MSM / pyBDY

pyBDY: a Python based regional NEMO model configuration toolbox.
GNU General Public License v3.0
7 stars 7 forks source link

#53 error catching to time interp #133

Closed rdPatmore closed 1 year ago

rdPatmore commented 1 year ago

Adding error handling for the time interpolation.

Closes #53

Tasks:

rdPatmore commented 1 year ago

This is a draft and is not ready for merge.

jpolton commented 1 year ago

This is a draft and is not ready for merge.

(FYI you can convert it to draft in the menu just below the suggested reviewers)

rdPatmore commented 1 year ago

Immerse tasks have been created as a new issue, so that this one can be closed. There are plenty of tasks that can be done under this banner, but it might be easier to tackle in small steps.

Improvements have been made to interp_time() with some error catching. I also found and removed some redundant, non-functioning interpolation code.

The interpolation could be streamlined. There could be a case for a follow-up issue on this.

rdPatmore commented 1 year ago

The second warning was not done. The task was based on a TODO in the code. The code noted a possible error case if the existing time-step was not a factor of 86400. I wasn't sure whether that was needed. I imagine it is ok for the code to interpolate from any length of timestep.

rdPatmore commented 1 year ago

A comment above mentions redundant code. Following a chat with James, this code was not redundant. This code is to be reinstated before pull request goes through.

The removed code was there to handle sub-daily source data. This sub-daily routine did not pass a test with dummy data and needs to be examined after reinstatement.

jpolton commented 1 year ago

I pressed the wrong button and closed the PR by mistake. Doh. Reopened it now.

I was going to query whether @rdPatmore you have the makings of a unit test, if you have constructed dummy data and know what is it supposed to do?

jdha commented 1 year ago

@jpolton just waiting for the template unit test from João and we'll open a new PR for the time interp unit test

rdPatmore commented 1 year ago

@jpolton @jdha @anwiseNOCL

This is ready for review. I have restored the original interpolation only. Additions of interpolation unit testing can become a new issue.

jdha commented 1 year ago

sorry @rdPatmore I've had a visitor all week - I will get back to pyBDY things either this afternoon or first thing Monday. Especially this PR as it would be good to get the time interp sorted and a unit test in place. Will be in touch!

jpolton commented 1 year ago

sorry @rdPatmore I've had a visitor all week - I will get back to pyBDY things either this afternoon or first thing Monday. Especially this PR as it would be good to get the time interp sorted and a unit test in place. Will be in touch!

@jdha since you started on this do you want to finish it. Or shall I review it?

jdha commented 1 year ago

@rdPatmore you can merge the PR now - sorry for the delay.