-
Notifications
You must be signed in to change notification settings - Fork 1
DEGA-279-numerical-attributes #127
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
Conversation
* manually bringing in parquet processing * big int * fixing bug with parquet matrix processing * fix index parsing bug * removed print and console logs * added mat as kwarg * Update src/celldega/viz/widget.py Co-authored-by: Copilot <[email protected]> * Update js/read_parquet/network_from_parquet.js Co-authored-by: Copilot <[email protected]> * implemnting astype change --------- Co-authored-by: Copilot <[email protected]>
more efficient hash for sparse data Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
This PR refactors metadata handling to support mixed numeric and categorical attributes, deprecates legacy JSON/network parameters, and enhances widget cleanup and deprecation warnings across Python and JavaScript.
- Rename
col_cats/row_catstocol_attr/row_attrand implementadd_mixed_attributes_to_node_info. - Introduce deprecation warnings for JSON export methods and the legacy
networkwidget parameter. - Add
close()cleanup hooks in widget classes and update JS to finalize deck instances.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/celldega/clust/matrix.py | Renamed category parameters, added mixed attribute logic |
| src/celldega/clust/utils.py | Added add_mixed_attributes_to_node_info helper |
| src/celldega/viz/widget.py | Added close() methods and deprecation warnings |
| tests/unit/test_clust/test_matrix.py | Updated signature and added numeric attribute test |
| js/widget.js | Refactored render cleanup, removed legacy network init |
| js/viz/matrix_viz.js | Routed all category layers through new set_cat_data |
| js/matrix/set_constants.js | Replaced category counts with attribute-based counts |
| js/matrix/cat_data.js | Unified categorical & numeric attribute rendering |
Comments suppressed due to low confidence (3)
tests/unit/test_clust/test_matrix.py:366
- [nitpick] This test covers numeric row attributes but doesn’t verify column attributes. Add assertions for
col_attrkeys inmat.vizand numeric data incol_nodesto ensure full coverage.
def test_numeric_attributes_in_viz(self) -> None:
js/widget.js:121
- Removing the fallback assignment for
networkmeansnetworkwill be undefined ifmat_parquetis not provided. Introduce a fallback, e.g.,network = model.get('network').
// let network = model.get('network');
js/widget.js:184
- The
renderfunction no longer returns thecleanupcallback at the end. Addreturn cleanup;after the message handler so the widget can be finalized properly.
});
|
You can test the numerical attribute in this notebook |
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.
LGTM - the Clustergram_Example notebook runs fine, and all unit tests are passing
No description provided.