Added string model parsing in clingo wrapper

[camcunningham]

A sort of addition onto my last commit - same theme. Needed to be able to create a model based on a string instead of a temp file. If there should be a better naming convention, feel free to change it up.

Again, if this functionality exists elsewhere, let me know!

[gfrances]

Thanks, @camcunningham ! The functionality looks good, but we try to avoid duplicating so much code whenever possible.

In this case, looks like the PR could be easily refactored e.g. with one single method with signature

def parse_model(*, symbol_mapping, filename=None, content=None)

then some check to make user the user provides filename XOR content, and then refactor all common code
into a method _parse_model(...) that takes a Python generator providing one line of the model at a time.

Do you think you'd have the time to refactor the PR along those lines?
Cheers,

[camcunningham]

@gfrances Absolutely! Makes a lot of sense - I'll make those changes tonight

[camcunningham]

@gfrances Made some of those changes. Ended up iterating through the lines without a generator - didn't know if I'd need to use one. If you still want that I'd be happy to refactor

[gfrances]

I suggested the use of a generator because it would avoid the need for loading the entire file in memory, which is what the call to readlines is now doing. Usually wouldn't be such a big deal, but here we should assume that the ASP models can get pretty large on problems where the grounding is huge. Let me make some closer suggestions in the code of the PR itself.

[camcunningham]

Makes sense - I'll make those changes. Thanks for the info on generators!

[camcunningham]

Hm. That's strange. It passed the checks before the swap to generators - did I do it wrong or is there potentially an issue with using them there?

[haz]

Seems to be issues with the grounding on the docs with the LP solver. E.g., https://travis-ci.com/github/aig-upf/tarski/jobs/510458691

@gfrances -- any chance there's issues with gringo installing on travis?

[gfrances]

Nope, it's just that this line is iterating over the characters of the filename, not over the contents of the filename. Should be something like:

    with open(filename, "r") as f:
        return _parse_model((l for l in f), symbol_mapping)

[codecov-commenter]

Codecov Report

Merging #110 (612f63d) into devel (afc61a9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel     #110   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files          47       47           
  Lines        3046     3046           
  Branches      114      114           
=======================================
  Hits         2991     2991           
  Misses         45       45           
  Partials       10       10           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afc61a9...612f63d. Read the comment docs.

[camcunningham]

Ah - that was dumb of me! Took your suggestion way too literally. Should be good to go now.

[haz]

Would this make it an iterator, or build the full list before calling the method? Might seem weird, but would just passing in f work? It's treated as lines in _parse_model, and may well be just the iterator you're looking for...

[gfrances]

The (...) syntax creates a generator, but you're totally right that passing in f should work as well;
likewise, passing in content.splitlines() below should work too.

[camcunningham]

@gfrances Any other changes you think need to be made?

[gfrances]

I would maybe just adopt the suggestion by Chris, passing f and content.splitlines(), but otherwise I think we're good to go?

[gfrances]

Thanks @camcunningham, @haz !