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

support nested models #9

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mikemhenry
Copy link
Contributor

Fixes the issue with nested models. Will add some tests and more documentation as well. The tl;dr is first we do a pass to convert the json to a dictionary, then we unitify things.

@mikemhenry
Copy link
Contributor Author

but let me know @mattwthompson what you think of this approach.

@mikemhenry
Copy link
Contributor Author

So this solves the first half of #8 but I will make another PR that optionally enforces a more strict unit check.

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Looks good so far, I might have some quibbles about performance that I want to tinker with. Could you add some tests around this behavior?

@mikemhenry
Copy link
Contributor Author

@mattwthompson I will be adding tests to this PR ASAP!
Is there anything else you would like to see? It is pretty old, we have been using it for awhile now in gufe and I would like to get it merged in sometime within the next two weeks.

@mattwthompson
Copy link
Member

Tests should be fine here, there might be some more steps to getting it in a release (though once it looks good the release process for this package is quick). I might want to tinker with large models to uncover any obvious performance bottlenecks, but that doesn't need to be included here. Eventually I'd like to be able to store i.e. JSONs of parametrized systems of millions of atoms. Is gufe using this for anything large?

I use this in Interchange now so it brushes up against parts of our stack that needs to be stable so I might want to run some integration tests before putting it into a release. The toolkit doesn't use this functionality, though, so I don't imagine any trouble ahead there.

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.

2 participants