search

NetLogo / Tortoise

Compiler and runtime engine for NetLogo models that runs in JavaScript 🐢
https://netlogoweb.org
Other
55 stars 27 forks source link

Add `import-world`, `export-world`, `export-plot`, and `export-all-plots` #198

Closed TheBizzle closed 6 years ago

TheBizzle commented 6 years ago

Umm... yeah... lots of stuff here.

What seems to me to be the most controversial thing—something that any developer on this project should be aware of—is that I have drastically changed TestModels, which has traditionally been the workhorse of our test suite. Now, it is even more of a workhorse, and hopefully not in a way that's seen as problematic.

Basically, instead of just running the models, it now does import-world/export-world roundtripping sorcery, as well. Roundtripping export-world and import-world won't catch too many bugs without running full models—but caught a ton with them!—and the most sensible way to roundtrip on full models seems to me to be leveraging the info that TestModels provides. I originally had that as a separate test suite, but it didn't make sense to me to have the model tests basically running twice (once with exporting and once without), since those tests already take forever when run just once. So now I've just mashed them together.

Essentially, TestModels will take even longer now, and it's not easy to disentangle the "pure" model-running from the roundtripping stuff, but I think that this is worth it, in that it provides very thorough testing for a major feature. Others might disagree, though.

Also probably worthy of some defense: There's a "TODO" item in there for adding the model name to the export. In order to get that, though, I need the compiler to provide me with the model name. That is being added in NetLogo/NetLogo#1547. Once that's merged, then I can add the model name over here. That said, I don't think it makes sense to wait for that before merging this PR; the "model name" line is largely ignored by NetLogo implementations and consumers of export .csv files alike.

mrerrormessage commented 6 years ago

👏 👏 👏 This looks awesome! I gave it a quick look-through and had a few comments, but I haven't (and probably won't) do a "full" review. Way to go Jason! 🎉

CamdenClark commented 6 years ago

Thanks for dealing with my spaghetti code and stupid questions! I hope something useful came out of all my work, even if most of the implementation doesn't survive. If I had to do GSoC again with what I know now, I'd definitely do some things differently. I'm really happy to see this implemented and hope to keep pushing the people I work with towards using NLW.

TheBizzle commented 6 years ago

No worries, Camden. I can't say that I even recall any stupid questions, and the early version of export-world that you put together definitely helped me to see the big picture more clearly and steer the design towards what it has become. Writing a desktop-compatible export-world is challenging in a lot of ways, and it had to be implemented in order to suss out a lot of those challenges.

Ultimately, the real treasure was the code we threw out along the way.™

LaCuneta commented 6 years ago

This all looks great to me. I'm especially happy about tests for import/export across all our test models!

Since the tests are superseding the old model tests, I ran a quick comparison between how they behaved before when something was different between headless and Tortoise and how they behave now and the difference was negligible; they're still totally fine to use for tracking down a difference between the systems, which is what matters to me.