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.

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

Reapply reproducibility improvements, add more unit tests #3451

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Nov 22, 2024

This PR reapplies some of the reproducibility improvements, and adds many more unit tests.

I think there are two remaining pieces:

  • Unit test data movement (reproducibility_tests/move_output.jl)
  • Unit test repro tests (reproducibility_tests/test_mse.jl), which happens in a subsequent process (so that artifacts are uploaded to buildkite regardless of the outcome of the reproducibility tests)
  • Rename reproducibility_tests/print_new_mse.jl -> reproducibility_tests/mse_summary.jl
  • Rename compute_mse.jl -> reproducibility_tests/reproducibility_tools.jl
  • Delete reproducibility_tests/test_reset.jl
  • Use environment flags to simplify opting-in to reproducibility tests.

@charleskawczynski charleskawczynski force-pushed the ck/more_repro_unit_tests branch 7 times, most recently from c6fb861 to 0a0c158 Compare November 25, 2024 14:08
@charleskawczynski
Copy link
Member Author

Ok, I think I realized the incremental path here. We can first add this infrastructure alongside our existing one & export the hdf5 data, then pull out the nc comparisons. That's what I'll do. I'll open a separate PR since this one is pretty entangled around the source code.

@charleskawczynski charleskawczynski force-pushed the ck/more_repro_unit_tests branch 3 times, most recently from e9e14fb to 20a86d7 Compare November 25, 2024 20:14
@charleskawczynski
Copy link
Member Author

I think that our exported NC data is exporting data correctly, and only captures a subset of the domain (this was afterall developed for a single column in mind).

I don't think we can incrementally change to a system that uses HDF5 data-- we would first need to fix the exported NC data, which seems a bit laborious to me. We could make some other incremental changes, but most of those would be name improvements, but some of them only make sense in this new context.

So, I think the right course of action is to take a leap onto this new system, which is significantly more tested than the existing one. Since I'm on vacation tomorrow, I'll probably try to merge this on December 18, instead.

The last thing needed is to increment the reference counter.

@charleskawczynski charleskawczynski force-pushed the ck/more_repro_unit_tests branch 6 times, most recently from 97c80f5 to 79e13f0 Compare December 19, 2024 19:41
@charleskawczynski
Copy link
Member Author

I think I'll do the environment variable business in a subsequent PR, since it's somewhat orthogonal to the existing logic.

@charleskawczynski charleskawczynski marked this pull request as ready for review December 19, 2024 19:41
@charleskawczynski charleskawczynski force-pushed the ck/more_repro_unit_tests branch 3 times, most recently from fd0dcc7 to ce84497 Compare December 19, 2024 20:52
@charleskawczynski charleskawczynski force-pushed the ck/more_repro_unit_tests branch 9 times, most recently from c56f2e0 to f2b1dc4 Compare December 22, 2024 13:32
debug

reproducibility_tests/print_new_mse.jl -> mse_summary

Increment ref counter
@charleskawczynski charleskawczynski merged commit b2b3e7f into main Dec 23, 2024
16 checks passed
@charleskawczynski charleskawczynski deleted the ck/more_repro_unit_tests branch December 23, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant