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

Convert Decimal type results to float for Gremlin #256

Merged
merged 8 commits into from
Feb 8, 2022
Merged

Conversation

michaelnchin
Copy link
Member

Issue #, if available: #254

Description of changes:

  • Fixed a issue where the presence of any Decimal type property values in Gremlin query results would prevent the graph widget from being displayed. GremlinNetwork now ensures that all Decimal type properties are converted to float before we attempt to generate the graph widget.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

krlawrence
krlawrence previously approved these changes Feb 6, 2022
Copy link
Contributor

@krlawrence krlawrence 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- did you do a test with the air routes data loaded by seed and a somewhat big query result? I'm curious if this impacts rendering performance much on either Gremlin Server or Neptune.

@krlawrence
Copy link
Contributor

We should probably add a change-log entry for this.

@michaelnchin
Copy link
Member Author

Added changelog entry. Tested performance of Gremlin network generation step on ~400 Air Routes results, found negligible (<1ms) difference in average duration between patched and non-patched version, on both Gremlin Server and Neptune.

Copy link
Contributor

@krlawrence krlawrence 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

@michaelnchin michaelnchin merged commit b5d1885 into main Feb 8, 2022
@michaelnchin michaelnchin deleted the decimal-debug branch February 8, 2022 00:31
charlesivie added a commit that referenced this pull request Feb 22, 2022
* added RDF ML notebooks

* cleaning up the code a bit

* completed regression and classification for SPARQL ML

* added RDF ML notebooks

* cleaning up the code a bit

* completed regression and classification for SPARQL ML

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Rename gremlin profile args for clarity (#249)

Co-authored-by: Michael Chin <[email protected]>

* Add --results-per-page query option (#242)

* Add -r option

* Replace interval value

* Add upper limit

Co-authored-by: Michael Chin <[email protected]>

* Pin ipython<7.17.0 to patch vulnerability (#250)

Co-authored-by: Michael Chin <[email protected]>

* Disable root logger output (#248)

Co-authored-by: Michael Chin <[email protected]>

* Fix OC Bolt metadata (#255)

* Fix OC Bolt query metadata output

* Add OC metadata tests

Co-authored-by: Michael Chin <[email protected]>

* Add groupby raw node result option (#253)

* Add --group-by-raw query option

* Add unit tests

Co-authored-by: Michael Chin <[email protected]>

* Add Gremlin group-by-depth (#251)

* Add --group-by-depth to Gremlin, modify OC with same explicit param

* Add tests

* PEP8 fixes

Co-authored-by: Michael Chin <[email protected]>

* addressing comment left in PR #252 related to better conditional statements

* Update ChangeLog.md

* Suffix all doubles with d. Node batch reduced to 40 (#257)

* Suffix all doubles with d. Node batch reduced to 40

* Update changelog for seed file changes

* Update ChangeLog.md

* Update ChangeLog.md

Co-authored-by: Michael Chin <[email protected]>

* Convert Decimal type results to float for Gremlin (#256)

* Convert Decimal type values in GremlinNetwork

* replace star import

* Revert temp test change

* Convert decimal type edge properties

* Update Changelog

Co-authored-by: Michael Chin <[email protected]>

* removed hard coded genre deletes, and moved them into the notebook.

* removed hard coded genre deletes, and moved them into the notebook.

* removed hard coded ciritcScore deletes, and moved them into the notebook.

* removed hard coded link prediction retractions, and moved them into the notebook.

* completed the Getting Started notebook with the pretrained models.

* completed the Getting Started notebook with the pretrained models.

* added RDF ML notebooks

* cleaning up the code a bit

* completed regression and classification for SPARQL ML

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* added RDF ML notebooks

* cleaning up the code a bit

* completed regression and classification for SPARQL ML

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* Complete notebooks for SPARQL Classification - Regression and Link Prediction

* addressing comment left in PR #252 related to better conditional statements

* removed hard coded genre deletes, and moved them into the notebook.

* removed hard coded genre deletes, and moved them into the notebook.

* removed hard coded ciritcScore deletes, and moved them into the notebook.

* removed hard coded link prediction retractions, and moved them into the notebook.

* completed the Getting Started notebook with the pretrained models.

* completed the Getting Started notebook with the pretrained models.

* tests passing for new SPARQL ML notebooks

* Added detail fo new SPARQL/RDF ML notebooks to the change log

* fixed bugs in link prediction, amde small improvements after review

* updated location of pre-trained models

Co-authored-by: Michael Chin <[email protected]>
Co-authored-by: Michael Chin <[email protected]>
Co-authored-by: Kelvin Lawrence <[email protected]>
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