Fix initial file read #579

LenkaNovak · 2024-01-29T16:14:24Z

closes #580

I have read and checked the items on the review checklist.

juliasloan25

I left a few comments! This piece of the code has always been a bit confusing to me so I want to make sure I totally understand the changes and that the tests are comprehensive before we merge

src/BCReader.jl

test/bcreader_tests.jl

test/mpi_tests/bcreader_mpi_tests.jl

juliasloan25 · 2024-01-30T19:02:50Z

Btw, our macOS github actions are failing right now because of a bug introduced by Plots.jl v1.40.0. There's an issue open in their repo about this here. Maybe for now we can pin to v1.39 and update it later

LenkaNovak · 2024-01-30T19:48:43Z

Btw, our macOS github actions are failing right now because of a bug introduced by Plots.jl v1.40.0. There's an issue open in their repo about this here. Maybe for now we can pin to v1.39 and update it later

Sounds good, nice catch! 🚀 Thank you for the review!

szy21 · 2024-01-30T21:32:23Z

I don't think I understand the logic of file reader enough, so maybe I can just ask you later. No need to wait for that to merge this PR though. Maybe it would be helpful to add some examples in the code comment, e.g.

# case 1: date is before the first date in the file, e.g. date: 19790210, file:19790215-19790315

(I'm not sure if this is exactly correct, but something like this)

src/BCReader.jl

juliasloan25 · 2024-01-31T02:19:44Z

Thank you for addressing the review comments, it's a lot more clear now! I like how you broke up the cases some more :) I had a few more questions/comments still

Project.toml

fix bc, clarify test fix bc, clarify test fix comments rev + split init cases rev2 rev3

LenkaNovak marked this pull request as ready for review January 30, 2024 03:52

LenkaNovak requested review from juliasloan25 and szy21 January 30, 2024 03:53

juliasloan25 reviewed Jan 30, 2024

View reviewed changes