Added I/O support for JSON Graph Format

[neumannrf]

List of changes:

1. This PR adds an I/O class called JSONGraphFormat that is able to .save() and .load() network geometry files in the JSON Graph Format.

2. The adherence to the format is enforced by means of .__validate_json__(), that checks the JSON file against the jgf_schema.pkl.

3. This validation functionality requires the jsonschema module, which I added to conda_requirements.txt.

4. Two unit tests were created for each of the JSONGraphFormat class methods: one for the success, one for the failure. Appropriate fixtures were created to support the tests. The new classes have 100% test coverage.

5. Bonus: I also added .editorconfig to help you standardise the code formatting across many developers and IDEs.

Closes #934

[codecov]

Codecov Report

Merging #975 into dev will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #975      +/-   ##
==========================================
+ Coverage   88.93%   89.02%   +0.09%     
==========================================
  Files          94       95       +1     
  Lines        8267     8337      +70     
==========================================
+ Hits         7352     7422      +70     
  Misses        915      915

[neumannrf]

@jgostick I am not 100% proud of my setup_class() method in JSONGraphFormatTest.

It feels like a dirty fix not no invoke op.geometry.GenericGeometry() as everyone else in tests/unit/io/*Test.py does. But, on the other hand, I did not want to mess around with more things than I absolutely have to, so I tried to keep it simple and direct.

Is that OK?

[ma-sadeghi]

@neumannrf Nice work 👍

[jgostick]

this makes me very happy. I'll take a close look at it on my flight home or sooner.

[neumannrf]

@jgostick How can I better keep the mergeability of this PR in the meantime? I see this is not expected to be looked at until v2.1 is released, during which period the master branch will diverge immensely from what I used as a baseline. Which branch should I keep merging into mine as to allow a conflict-less PR merge?

[jgostick]

I don't think it will actually change that much. We just need to get the V2.0 stable, then can start adding features. The master branch is currently V2.0-beta, and the dev branch is where we are merging new PRs. Every now and then we merge dev in to master and update the version number. So, if you keep yours up to date then just keep merging master into it.

[neumannrf]

@jgostick Now that the first PRs tagged as V2.1 started being merged, I think it's the right time to sync with upstream/dev and put this PR in a mergeable state for your consideration.

Please let me know if there is any required adjustment. I assume, since I only added new files, there should be no conflicts with the changes that the code has been subject to in the recent months.

[jgostick]

Hi Rodrigo! Yes, we are just about there. It is number 2 on my to do list, after updating all the examples to version 2! I this happening next week. I will take a look at your code again, and get back to you with comments. Because it's a PR from your fork I'm not able to edit it (unless I fork your code and make a PR to YOUR repo, but that get confusing!). So talk to you early next week.

[jgostick]

This code is great! The only thing I commented on was the need to allow phases to be saved as well as just network data. This is not too hard to do for save but is a pain if you want to load phase data. Actually, I'm not really sure how the jsongraph format works...maybe it won't like the phase data. For instance, how do you save 'pore.temperature' for two different phases since they both have the same name? We we save to VTK, for example, we do 'water.pore.temperature' and 'air.pore.temperature' (actually we replace the dots by pipes ('air | pore | temperature'), but I'm guessing this doesn't work with jsongraph format. Maybe insert the object name before the property name, like 'pore.air_temperature'?

[neumannrf]

Hi @jgostick! Thanks for your detailed review and kind words of encouragement!
I'm out of office this week, but I'll address all the requested changes next week when I'm back. Once I perform the requested modifications I'll give you a heads up for your approval.

[jgostick]

If this phase business ends up being a pain, then forget it and we can live with topology io only. We could perhaps add a note to the docstring to mention that users must transfer any phase data to the network manually if they wish to keep it.

[neumannrf]

@jgostick I went over the proposed changes and performed the modifications (wherever applicable). I hope my last two commits address the points you raised appropriately. In my replies to your comments you will see a summary/explanation of the latest commits.

Please let me know if there is any pending fix that prevents the PR from being merged. I hope the PR looks good now! 🤞

[neumannrf]

I am a little bit puzzled by the outcome of the AppVeyor and Codecov checks. 🤔 😱

Apparently, the tests in JSONGraphFormatTest.py were successful, but there were 4 errors in the example_script.py and example_script_two_subdomains.py scripts (which I did not touch). Some of this failed tests are already present in the dev branch from which branched mine. Also the 24% coverage reported there is different from the 100% score achieved in the last Travis build.

Did I miss some huge change in the CI workflow that has caused all my (previously flawless) checks to fail?

[jgostick]

Ignore the appveyor checks...we added those to check windows builds, but this hasn't worked as planned and we'll be removing them. I see that you merge dev into your branch....but I'm not sure why travis didn't even build. Github was having trouble a few nights ago, so maybe something isn't fixed quite yet?

[jgostick]

btw if you go to travis your PR did pass...not sure if you can see this without being logged in, but this is my link

[jgostick]

Can you try tweaking something and repushing? That should trigger a new build that will hopefully work.

[neumannrf]

Can you try tweaking something and repushing? That should trigger a new build that will hopefully work.

Done! 👍
The Codecov check did not kick in yet, but the Travis CI build already confirms the 100% coverage of the new IO class.