Skip to content

Checking in IO protocols and readers#3

Merged
piyushrpt merged 12 commits intoisce-framework:mainfrom
piyushrpt:feat/io
Apr 3, 2024
Merged

Checking in IO protocols and readers#3
piyushrpt merged 12 commits intoisce-framework:mainfrom
piyushrpt:feat/io

Conversation

@piyushrpt
Copy link
Contributor

  • IO protocols
  • Basic raster based reader to read test data

@piyushrpt piyushrpt marked this pull request as ready for review March 30, 2024 00:53
@piyushrpt piyushrpt requested review from gmgunter and mirzaees March 30, 2024 00:54
Copy link
Member

@gmgunter gmgunter left a comment

Choose a reason for hiding this comment

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

Good stuff, thanks. Few questions and suggestions below.

Apologies in advance if some comments are outdated -- looks like some updates came in as I was finishing up my review.

@gmgunter
Copy link
Member

gmgunter commented Apr 2, 2024

Checking in Delaunay triangulation

Sorry, can you revert this commit and put it in a separate PR? If you're comfortable with git rebase, you can chain the PR branches on top of each other, then rebase the newer PRs on top of the main branch as the older PRs get merged.

I concede that that's a lot more burdensome on you when the PRs have dependencies on each other, but it makes my life as a reviewer a lot easier. I can get through a bunch of smaller PRs much more efficiently than one big PR.

No worries if reverting the commit is a problem this time, but going forward let's try to figure out a workflow that involves chaining multiple smaller PRs, since that's the only way that I'll realistically be able to continue reviewing them.

@piyushrpt
Copy link
Contributor Author

Ignore the Delaunay files for now. Moving forward, I will daisy chain the PRs. The next PR will be Delaunay related and we can discuss that in more detail then.

@piyushrpt piyushrpt requested a review from gmgunter April 2, 2024 15:34
Copy link
Member

@gmgunter gmgunter left a comment

Choose a reason for hiding this comment

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

Big fan of the updated docstrings. It's a lot more clear what's going on now, thanks.

Just a heads up -- if I add more than like 10 review comments at once, GitHub collapses some of the comments and you need to click on [Load more...] to reveal them. You might not have seen some of my earlier comments because of this.

image

Moving forward, I will daisy chain the PRs.

Thanks! I really appreciate it.

@piyushrpt piyushrpt requested a review from gmgunter April 2, 2024 23:20
Copy link
Member

@gmgunter gmgunter left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all of my questions and comments

@piyushrpt piyushrpt merged commit 3e26a20 into isce-framework:main Apr 3, 2024
@piyushrpt piyushrpt deleted the feat/io branch April 3, 2024 17:00
@gmgunter gmgunter mentioned this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants