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.

Sign up for GitHub

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

Jump to bottom

Add additional Model convenience methods #119

Merged
merged 17 commits into from
Dec 15, 2022
Merged

Add additional Model convenience methods #119

merged 17 commits into from
Dec 15, 2022

Conversation

adamnsch
Copy link
Contributor

@adamnsch adamnsch commented Jun 2, 2022

No description provided.

@adamnsch adamnsch marked this pull request as ready for review June 2, 2022 12:13
@Mats-SX Mats-SX self-assigned this Jun 2, 2022
Mats-SX
Mats-SX requested changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@Mats-SX Mats-SX left a comment

Choose a reason for hiding this comment

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

I think we should discuss the name of pipeline and what other convenience methods we can add. Maybe we want to call it pipeline_steps for example.

changelog/1.1.0.md Outdated Show resolved Hide resolved
graphdatascience/model/pipeline_model.py Outdated Show resolved Hide resolved
@adamnsch adamnsch marked this pull request as draft June 2, 2022 12:33
@Mats-SX Mats-SX marked this pull request as ready for review December 12, 2022 09:10
@netlify
Copy link

netlify bot commented Dec 12, 2022 •
edited
Loading

✅ Deploy Preview for neo4j-graph-data-science-client canceled.

Name Link
🔨 Latest commit ffaf6d9
🔍 Latest deploy log https://app.netlify.com/sites/neo4j-graph-data-science-client/deploys/639b1ee38c0e1d0008b87623

@Mats-SX Mats-SX removed their assignment Dec 13, 2022
@FlorentinD FlorentinD self-assigned this Dec 14, 2022
FlorentinD
FlorentinD approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@FlorentinD FlorentinD left a comment

Choose a reason for hiding this comment

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

LGTM

Some smaller remarks only ;)

changelog.md Outdated Show resolved Hide resolved
doc/modules/ROOT/pages/pipelines.adoc Show resolved Hide resolved
doc/modules/ROOT/pages/pipelines.adoc Show resolved Hide resolved
graphdatascience/model/link_prediction_model.py Show resolved Hide resolved
graphdatascience/model/link_prediction_model.py Show resolved Hide resolved
graphdatascience/model/model.py Show resolved Hide resolved
graphdatascience/model/pipeline_model.py


@dataclass(repr=True)
class EvaluationScores(Dict[str, float]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I really hope we keep this format on the server side in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider to align the naming between server and client? On the server this is ModelStats

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's too risky to replicate this client-side? I figured that this metric triple would remain stable, but yes it is a bit of a bet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda prefer EvaluationScores to ModelStats so maybe a refactoring server-side would be my preference. What do you think?

It's not part of the API, I believe. It's an anonymous map/dict in a list mapped to some metric name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also like EvaluationScores more

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not part of the external API but for our team it would be better to use the same names IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I will push a commit to master

graphdatascience/pipeline/training_pipeline.py Outdated Show resolved Hide resolved
adamnsch and others added 14 commits December 15, 2022 09:13
@adamnsch @FlorentinD @Mats-SX
Add NCModel.classes convenience method
fff871b 
Co-Authored-By: Florentin Dörre <florentin.dorre@neotechnology.com>
@adamnsch @Mats-SX
Add Model.model_info convenience method
0ec945b
@adamnsch @Mats-SX
Give pipeline models new convenience method best_parameters
73e487c
@adamnsch @Mats-SX
Give pipeline models new convenience method pipeline
0290a3b
@adamnsch @Mats-SX
Add changelog entries for new model convenience methods
ff63dc2
@Mats-SX @adamnsch
Fix pandas import
a48aa1b 
Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Add simple factory methods to create pipelines
0340ba8 
Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Use snake case
ddd7ddb 
Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Override metrics() for pipeline models
860afca 
This now returns a custom dataclass object for easier development.

Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Make MetricScores and EvaluationScores dicts
a5fc858 
To avoid breaking the API

Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Fix typing errors
c5682e1 
Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Ignore all venvs for flake8
c97c8db 
Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Add additional functions for pipeline models
a42c038 
- feature_properties
- link_features
- node_property_steps

Also make TrainingPipeline generic in its produced model type.

Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
@Mats-SX @adamnsch
Inline the RegressionTrainingPipeline
3ab4a03 
It was abstract with a single inheritor

Co-authored-by: Adam Schill Collberg <adam.schill.collberg@protonmail.com>
Mats-SX and others added 2 commits December 15, 2022 09:15
@Mats-SX @FlorentinD
Improve changelog
abf3847 
Co-authored-by: Florentin Dörre <florentin@owitsch.de>
@Mats-SX @FlorentinD
Improve type var name
781d89d 
Co-authored-by: Florentin Dörre <florentin.dorre@neotechnology.com>
@Mats-SX
Remove typing ignore comment
ffaf6d9 
This is checked/not checked dependening on the version of Python/mypy
@Mats-SX Mats-SX merged commit 8e60c19 into neo4j:main Dec 15, 2022
@adamnsch adamnsch deleted the model-convenience branch December 15, 2022 13:47
@FlorentinD FlorentinD mentioned this pull request Jan 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mats-SX Mats-SX Mats-SX approved these changes

@FlorentinD FlorentinD FlorentinD approved these changes

Assignees

@FlorentinD FlorentinD

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamnsch @Mats-SX @FlorentinD