search

TerrainBento / terrainbento

A modular landscape evolution modeling package built on top of the Landlab Toolkit.
https://terrainbento.readthedocs.io/
MIT License
19 stars 8 forks source link

Advanced output writers. #155

Closed alexmitchell closed 3 years ago

alexmitchell commented 3 years ago

Answers issue #154

All new advanced output writer system. The biggest benefit is that it allows multiple output writers to write output at independent and arbitrary times. The system is fully backwards compatible, so existing model code should have no problem running with the new output framework. Please see the updated notebook on output writers for more information and examples.

All of the new code is thoroughly (and hopefully rigorously) tested with 100% coverage and all passes. It should be noted that there are 30 failures that predated any changes I made and are therefore unrelated to this new output system. I'll make a new issue for this.

The new output writer code hooks directly into ErosionModel through various new methods and attributes. However, in retrospect, it would be cleaner to pull all the output related code from ErosionModel and put it in a new OutputWriterManager. I'd imagine this would be a fast change to make in general, but I did not do this because I fear writing MORE testing code and I ran out of time for working on this tangential project.

Also, this is the first time I have created a pull request for a major project (i.e. with multiple participants and quality standards). Please let me know if there is anything else I need to do.

kbarnhart commented 3 years ago

@alexmitchell thanks for this PR. I'll take a look at it and let you know if I have any comments. I won't be able to do this in the next few days but should be able to do this by the end of the week.

Will also look at #156. I should be able to fix that.

🎉

gregtucker commented 3 years ago

What a great new capability!

kbarnhart commented 3 years ago

@alexmitchell at first glance this looks like really high quality pull request for a great set of features! Really nice work. I still should be able to get you comments by the end of this week. And addressing #156 won't be a block to merging this.

🎉 🎉 🎉

kbarnhart commented 3 years ago

@alexmitchell I've now taken a close look through this and WOW! this is a great contribution. I've made a few changes, mostly things to get your new features into the documentation. And to get the docs to build without issue (here, you again deserve high praise as there were very few sphinx formatting issues 🎉 🎉 🎉 )

One thing that I thought should be emphasized a bit more in the documentation is that it is a user's responsibility to make sure that the model time steps line up to hit the the output time points (and that there will first be warnings, then an error). I think the way you've handled this is good. So I added a note about that in a couple of places and on the output writer page.

Let me know if you have any issues with anything I've changed. If not, I'll go ahead and merge this.

Thanks again for making such a great set of features, testing them, ensuring backwards compatibility, etc. I really like the design you've implemented in which most of the functionality is in the GenericOutputWriter.

alexmitchell commented 3 years ago

Thanks for the feedback. Looks like whitespaces were the worst issue.... Phew! My text editor of choice autotabs twice for unclosed parentheses, brackets, etc. . I tried to fight it I swear!

Docs are magic to me at this point. I make the triple quote string and somehow that turns into a web page. I'm glad there weren't too many issues there!

I'm okay with all the changes. Cheers :tada: :tada: :tada: We'll see how it performs out in the wild.

kbarnhart commented 3 years ago

Thanks @alexmitchell ! Really nice work!