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

Representation params #41

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Representation params #41

wants to merge 5 commits into from

Conversation

sbittrich
Copy link
Member

Description

Basic support for representation-specific parameters.

Any other params that we want to support at this point?

Should we add some validation? Things like component.representation(type="cartoon", ignore_hydrogens=True) can be done, but ignore_hydrogens won't be propagated to the final JSON.

Actions

  • Added description of changes to the [Unreleased] section of CHANGELOG.md
  • (Optional but encouraged) Added example(s) for new feature(s) in this PR to app/api/examples.py

@sbittrich
Copy link
Member Author

@dsehnal Is this the right direction?

Copy link
Member

@dsehnal dsehnal left a comment

Choose a reason for hiding this comment

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

Yeah, this is what I had in mind.

Comment on lines 441 to 444
class CartoonParams(RepresentationParams):
size_factor: Optional[float] = Field(description="Scales the corresponding visuals.")
tubular_helices: Optional[bool] = Field(description="Simplify corkscrew helices to tubes.")
# TODO support for variable size, e.g. based on b-factors?
Copy link
Member

@dsehnal dsehnal Dec 20, 2024

Choose a reason for hiding this comment

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

Adding type: Literal["cartoon"] = "cartoon" here would help with the issue you mentioned, where you would not be able to mix properties for different kinds (although, for that, might have to add class Config: extra = "forbid" to the pydantic model).

Copy link
Member Author

Choose a reason for hiding this comment

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

Looked into this and updated make_params to raise an exception if there are "unconsumed" properties (a9b471b). Doesn't affect other examples and, at least, gives users an error rather than silently ignoring potential mistakes.

Comment on lines 457 to 461
RepresentationTypeParams: Dict[RepresentationTypeT, Type[Union[CartoonParams, BallAndStickParams, SurfaceParams]]] = {
"cartoon": CartoonParams,
"ball_and_stick": BallAndStickParams,
"surface": SurfaceParams,
}
Copy link
Member

Choose a reason for hiding this comment

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

Like the dictionary approach. Maybe can do this too so the type doesn't have to be repeated:

Suggested change
RepresentationTypeParams: Dict[RepresentationTypeT, Type[Union[CartoonParams, BallAndStickParams, SurfaceParams]]] = {
"cartoon": CartoonParams,
"ball_and_stick": BallAndStickParams,
"surface": SurfaceParams,
}
RepresentationTypeParams = {
t.__fields__["type"].default: t for t in (CartoonParams, BallAndStickParams, SurfaceParams)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, moved to that impl with 9a46576.

@sbittrich sbittrich marked this pull request as ready for review December 23, 2024 13:10
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