Closed camcunningham closed 3 years ago
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,
@gfrances Absolutely! Makes a lot of sense - I'll make those changes tonight
@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
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.
Makes sense - I'll make those changes. Thanks for the info on generators!
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?
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?
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)
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.
Ah - that was dumb of me! Took your suggestion way too literally. Should be good to go now.
@gfrances Any other changes you think need to be made?
I would maybe just adopt the suggestion by Chris, passing f
and content.splitlines()
, but otherwise I think we're good to go?
Thanks @camcunningham, @haz !
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!