Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial outline of reference implementation. #26

Merged
merged 2 commits into from
Jan 13, 2021

Conversation

jeromekelleher
Copy link
Member

WIP putting together a reference implementation in Python. The terms-of-reference for this are still coming together in my head as it develops, but the current idea is to make it a strict but unhelpful parser (i.e., don't expect nice error messages).

I'm avoiding using JSON schema as I want to implement it as someone reading the spec without having a JSON schema implementation to lean on.

There's a question about how much CI etc we do with this. My vote would be to keep it fairly minimal.

@grahamgower
Copy link
Member

Looks good so far!

There's a question about how much CI etc we do with this. My vote would be to keep it fairly minimal.

I think it will be useful for the demes-spec repository to include a set of test yaml (or json) files. There would be two sets, one for valid input , and one invalid input. The intention is for these to be available for the development/testing of new implementations. This reference implementation could then be tested against those files.

@jeromekelleher jeromekelleher marked this pull request as ready for review December 10, 2020 23:42
@jeromekelleher
Copy link
Member Author

It would be good to get some input on this here @grahamgower, @apragsdale, @molpopgen. There's still a fair bit to be done, but I think the general outline of how we do the "hard" bits is reasonably clear. It took a few attempts to boil it down to this.

I'm hoping the code is clear, so I won't explain it. Start at the Graph.parse method - any input much appreciated!

Copy link
Member

@apragsdale apragsdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great so far! This was really useful to read through, and will certainly help also with cleaning up the demes-python implementation I think.

I suppose the next tricky part will be figuring out everything that belongs in graph.validate(). I really like how you've put together the default attribute inheritance here.

examples/stepping_stone_model.yml Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
Copy link
Member

@grahamgower grahamgower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very clean, definitely we should port stuff into demes-python where it's easy to do so.

reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
@molpopgen
Copy link
Contributor

I'm still going over the details, but this looks great!

@jeromekelleher
Copy link
Member Author

I think this is ready for final review and merging now. There's a few loose ends to tidy up and we need to add in a lot more examples (see #27), but overall I think it's fairly solid. I suggest we merge this fairly soon, and then follow up with the corresponding updates to the JSON schema, spec wording, etc. We should also follow up to add basic CI too (basically, run tests, check that coverage is 100% [no need for codecov, etc, hopefully], and run mypy).

@jeromekelleher jeromekelleher force-pushed the ref-impl branch 2 times, most recently from 6ff1831 to f363b0c Compare December 29, 2020 22:21
@jeromekelleher
Copy link
Member Author

Update here: I've added a bunch of example files like examples/x.yml and examples/x.resolved.json. These are supposed to be examples of an input yaml file and the output fully qualified JSON, which should be a good resource for parser writers. It'd be nice to build this out into a sort of "compliance suite" by adding more examples, although I guess we'd need to add a bunch of files that provoke errors too to do it properly.

Anyway, don't bother going through the .resolved.json files too much, as they are auto generated.

@jeromekelleher jeromekelleher mentioned this pull request Dec 30, 2020
Copy link
Member

@grahamgower grahamgower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @jeromekelleher! Just some minor comments below.

reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
reference_implementation/tests.py Outdated Show resolved Hide resolved
reference_implementation/parser.py Outdated Show resolved Hide resolved
@jeromekelleher
Copy link
Member Author

Great, thanks @grahamgower! I've tacked on a few commits to deal with these and opened issues for the stuff that's a bit more work. I'll squash these before final merge.

@molpopgen, @apragsdale, are you happy to merge this?

@molpopgen
Copy link
Contributor

Happy to merge

@jeromekelleher
Copy link
Member Author

@apragsdale?

@apragsdale
Copy link
Member

Yes, happy to merge - nothing jumps out to me that I could see!

@jeromekelleher
Copy link
Member Author

Great! All squashed and ready to go - I'll let you push the merge button @grahamgower.

@jeromekelleher jeromekelleher merged commit 56792de into popsim-consortium:main Jan 13, 2021
@jeromekelleher jeromekelleher deleted the ref-impl branch January 13, 2021 07:15
@jeromekelleher
Copy link
Member Author

(I merged this as I want to get on with #40)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants