-
Notifications
You must be signed in to change notification settings - Fork 4
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
basic raw volumetric data support #35
base: master
Are you sure you want to change the base?
basic raw volumetric data support #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice.
I have a few doubts which I mentioned in comments.
|
||
source: RawVolumeSourceT = Field(description="The type of the raw input file with volumetric data.") | ||
options: RawVolumeOptionsT = Field(description="Specifies the voxel size and mapping of sequential channel IDs to user-defined channel IDs.") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep the param hierarchy flat (without nesting) - it will be much easier to process params in frontend. Also it will allow adding description
to each field separately.
I.e.:
source: RawVolumeSourceT = Field(description="The type of the raw input file with volumetric data.")
voxel_size: float | None = Field(...)
channel_ids_mapping: ChannelIdsMapping | None = Field(...)
Representation node, describing how to represent a volume. | ||
""" | ||
# TODO: add slice | ||
type: VolumeRepresentationTypeT = Field(description="Representation type, i.e. isosurface or direct_volume") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose eventually we will have more params here, e.g. isovalue, slice plane. But if this is just WIP, it's fine to add them later.
molviewspec/molviewspec/builder.py
Outdated
params = make_params(RawVolumeParams, locals(), type="raw_volume") | ||
node = Node(kind="raw_volume", params=params, additional_properties=additional_properties) | ||
self._add_child(node) | ||
return RawVolume(node=node, root=self._root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we call it "raw volume". Wouldn't just "volume" be enough? Do we need to contrast it with some other type of volume which is less raw?
To me "raw volume" sounds a bit like the volumetric data are included directly in MVS, instead of being read from a referenced file.
This applies to multiple classes, methods, type names.
molviewspec/molviewspec/builder.py
Outdated
# params = make_params(ColorFromUriParams, locals()) | ||
# node = Node(kind="color_from_uri", params=params, additional_properties=additional_properties) | ||
# self._add_child(node) | ||
# return self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove dead code
def color( | ||
self, | ||
*, | ||
color: ColorT, | ||
selector: ComponentSelectorT | ComponentExpression | list[ComponentExpression] = "all", | ||
additional_properties: AdditionalProperties = None, | ||
) -> Representation: | ||
""" | ||
Customize the color of this representation. | ||
:param color: color using SVG color names or RGB hex code | ||
:param selector: optional selector, defaults to applying the color to the whole representation | ||
:param additional_properties: optional, custom data to attach to this node | ||
:return: this builder | ||
""" | ||
params = make_params(ColorInlineParams, locals()) | ||
node = Node(kind="color", params=params, additional_properties=additional_properties) | ||
self._add_child(node) | ||
return self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think selector
makes sense here, as we have no way of applying our current selectors to volumes.
At least for now.
In the future we might add different types of selectors, which would apply to volumes.
No description provided.