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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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
  • Improve compute_mse.jl filename
  • Delete reproducibility_tests/test_reset.jl
  • Add "integration test" where all pieces are used together
  • 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.

debug

reproducibility_tests/print_new_mse.jl -> mse_summary
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