-
Notifications
You must be signed in to change notification settings - Fork 25
Typed objects qol #202
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
base: main
Are you sure you want to change the base?
Typed objects qol #202
Conversation
| extra_content, | ||
| '</div>' | ||
| ] | ||
| return ''.join(html_parts) |
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.
| border-radius: 3px; | ||
| padding: 16px; | ||
| margin: 8px 0; | ||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, sans-serif; |
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.
Reviewing if this needs to align with evo branding or we use the fonts more common in the notebooks.
…typed objects from Evo. THe user does not need to know about the type.
…ng in objects too.
grantroch
left a comment
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 the Annotation/Schema changes but looked at everything else.
| raise DataLoaderError("Data was modified since the object was downloaded") | ||
| return await self._obj.download_dataframe(self.as_dict(), fb=fb, column_names=self.data_columns) | ||
|
|
||
| async def to_dataframe(self, fb: IFeedback = NoFeedback) -> pd.DataFrame: |
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 think we need to consider this change. We shouldn't have both of these methods around. Decide on one and move on. It seems like Ben already had conversations around this naming and perhaps they already considered to vs get.
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.
We should decide the naming of these methods and pick one convention. I changed from as to get, as the former was brought up as confusing in a code review. I'm not opposed to to.
| return await self.from_reference(self._context, self.metadata.url) | ||
|
|
||
| def search(self, expression: str) -> Any: | ||
| """Search the object metadata using a JMESPath expression. |
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.
Some more useful examples could be helpful here. I don't really want to lookup what a JMESPath expression. is.
| # Build unified table with all rows | ||
| table_rows = [] | ||
| for label, value in rows: | ||
| if label in ("Bounding box:",) or label.endswith(":") and isinstance(value, str) and "<table" in value: |
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.
The "Bounding Box" check is redundant with the endswith check. Also, I don't know if those are necessary as we didn't use to have those first two checks, only the latter checks.
| :param fb: Optional feedback object to report download progress. | ||
| :return: A DataFrame with cell attribute columns. | ||
| """ | ||
| if keys: |
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.
Do we really need this branch? We should support passing no keys as an option.
| :param fb: Optional feedback object to report download progress. | ||
| :return: A DataFrame with cell attribute columns. | ||
| """ | ||
| if keys: |
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.
Likewise, these shouldn't require this branch.
| :param fb: Optional feedback object to report download progress. | ||
| :return: A DataFrame with cell attribute columns. | ||
| """ | ||
| if keys: |
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.
Same here.
|
|
||
|
|
||
| @property | ||
| def viewer_url(self) -> str: |
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.
Can these be replaced by the shared functions in evo/common/urls?
| raise DataLoaderError("Data was modified since the object was downloaded") | ||
| return await self._obj.download_dataframe(self.as_dict(), fb=fb, column_names=self.data_columns) | ||
|
|
||
| async def to_dataframe(self, fb: IFeedback = NoFeedback) -> pd.DataFrame: |
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.
We should decide the naming of these methods and pick one convention. I changed from as to get, as the former was brought up as confusing in a code review. I'm not opposed to to.
| return await object_from_reference(context, reference) | ||
|
|
||
|
|
||
| class _BaseObject: |
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.
Was there a reason this no longer inherits from SchemaModel? This seems to be duplicating some logic from there.
| return await object_type._replace(context, reference, data, create_if_missing=True) | ||
|
|
||
|
|
||
| class ConstructableObject(BaseObject, Generic[_T]): |
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.
This doesn't appear to be used.
I initially used this class so that the data parameters to the create/replace/create_or_replace methods had the correct and strict type. i.e. for PointSet, the 'data' parameters would be type PointSetData, so passing a Regular3DGridData, could be picked up by a type checker. I subsequently decided to remove it, for simplification.
| if TYPE_CHECKING: | ||
| pass # Attributes is imported directly above |
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.
Remove
| if isinstance(value, str): | ||
| return value | ||
|
|
||
| # Check for ObjectReference (it's a str subclass with extra attributes) |
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.
Wouldn't it already be returned above, as isinstance(value, str) would be true even for a sub-class.

The first set of changes to Evo typed objects based on user interviews.
to_dataframe(which effectively is an alias forget_dataframe).jsonlet them be and add it for them.Instead of
pointset = await PointSet.from_reference(manager, reference)you cand dopoinset = await object_from_reference(manager, reference)