Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Boundary Condition Reader interface #175

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Boundary Condition Reader interface #175

merged 1 commit into from
Jan 3, 2023

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Nov 30, 2022 •
edited
Loading

For reviewers: This is a large PR, but the majority of the changes are in src/BCReader.jl, test/bcreader_tests.jl, and test/mpi_tests/bcreader_mpi_tests.jl. Also, I created the src/CallbackManager.jl file in this PR to simplify compatibility with BCReader, but that module will be documented and tested in a separate PR.

Move functions from coupler_utils/bcfile_reader.jl to src/BCReader.jl module, and ensure that they are thoroughly commented and tested.

#134

To-do

PHASE 1 - moving and documentation

  • Copy file(s) to module in src/
  • Add module to ClimaCoupler.jl
  • Docstring for each function
    • Add input/output types in docstring if not type annotated
    • Add docstring info to docs/src, docs/make.jl
  • Add exports for externally-used functions
  • Flag potentially-removable parts of code

PHASE 2 - testing

  • Create test file in test/, add to test/runtests.jl
  • Unit test for each exported function in module
  • Integration tests as needed
  • Add any tests that use MPI in test/mpi_tests and in buildkite pipeline

PHASE 3 - cleanup

  • Remove flagged parts of code
  • Check all imports are necessary
  • Formatting
  • Read through comments

@juliasloan25 juliasloan25 mentioned this pull request Dec 3, 2022
Closed
13 tasks
charleskawczynski
charleskawczynski reviewed Dec 6, 2022
View reviewed changes
src/BCReader.jl Outdated Show resolved Hide resolved
charleskawczynski
charleskawczynski reviewed Dec 6, 2022
View reviewed changes
src/BCReader.jl Outdated Show resolved Hide resolved
LenkaNovak
LenkaNovak reviewed Dec 8, 2022
View reviewed changes
Copy link
Collaborator

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

Looks good! I had a couple of comments and agree with Charlie's suggestions. Also, it would be good to implement this in the coupler_driver_modular.jl file once the #167 is merged.

test/bcreader_tests.jl Show resolved Hide resolved
test/bcreader_tests.jl
dummy_data = (; test_data = zeros(axes(land_mask_t)))

datafile_rll = sst_data
varname = "SST"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this using the original file with all of the SST dates? This would make the test quite slow. I wonder if we use a more lightweight version (e.g. save only a couple of the dates in a separate file in the Box as here).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a great idea. Do you know how we can select a subset of the dates in the file when or after we download it?

charleskawczynski
charleskawczynski reviewed Dec 10, 2022
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

This is a pretty big PR, could it maybe be broken into smaller parts?

Also, I suggest we start using SafeTestsets.jl in the test suite to avoid leaky tests-- this can cause some really unintuitive errors

test/bcreader_tests.jl Outdated Show resolved Hide resolved
@juliasloan25 juliasloan25 self-assigned this Dec 12, 2022
@juliasloan25 juliasloan25 force-pushed the js/bcreader_interface branch 2 times, most recently from a3a9963 to c45c04e Compare December 13, 2022 00:01
@juliasloan25 juliasloan25 mentioned this pull request Dec 13, 2022
Closed
@juliasloan25 juliasloan25 force-pushed the js/bcreader_interface branch 2 times, most recently from 2b76562 to 29378b2 Compare December 15, 2022 05:35
@juliasloan25 juliasloan25 marked this pull request as ready for review December 15, 2022 16:34
charleskawczynski
charleskawczynski reviewed Dec 15, 2022
View reviewed changes
src/BCReader.jl Outdated Show resolved Hide resolved
charleskawczynski
charleskawczynski reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

I left a comment about a performance concern.

valeriabarra
valeriabarra reviewed Dec 15, 2022
View reviewed changes
src/BCReader.jl Outdated Show resolved Hide resolved
src/CallbackManager.jl Show resolved Hide resolved
@juliasloan25 juliasloan25 force-pushed the js/bcreader_interface branch 2 times, most recently from d2a2765 to 46daf05 Compare December 22, 2022 04:51
charleskawczynski
charleskawczynski reviewed Dec 22, 2022
View reviewed changes
src/BCReader.jl Outdated Show resolved Hide resolved
@juliasloan25 juliasloan25 force-pushed the js/bcreader_interface branch 2 times, most recently from c29fb3f to 2c1ed02 Compare December 22, 2022 22:12
@juliasloan25 juliasloan25 force-pushed the js/bcreader_interface branch 2 times, most recently from b21321e to 79bc896 Compare December 23, 2022 20:42
@juliasloan25 juliasloan25 changed the title Boundary Condition Reader interface (WIP) Boundary Condition Reader interface Jan 3, 2023
@juliasloan25
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 3, 2023

Build succeeded:

@bors bors bot merged commit 7eb6902 into main Jan 3, 2023
@bors bors bot deleted the js/bcreader_interface branch January 3, 2023 16:22
This was referenced Jan 3, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LenkaNovak LenkaNovak LenkaNovak left review comments

@charleskawczynski charleskawczynski charleskawczynski left review comments

@valeriabarra valeriabarra Awaiting requested review from valeriabarra

Assignees

@juliasloan25 juliasloan25

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juliasloan25 @charleskawczynski @LenkaNovak @valeriabarra