457 geovex model saving loading #460
Conversation
Co-Authored-By: Muhammad Hassan <[email protected]>
Co-Authored-By: Muhammad Hassan <[email protected]>
…d loading of model clean up embedder load/save clean up tests Co-Authored-By: Muhammad Hassan <[email protected]>
Co-Authored-By: Muhammad Hassan <[email protected]>
for more information, see https://pre-commit.ci
…ving-loading' into 457-geovex-model-saving-loading
Co-Authored-By: Muhammad Hassan <[email protected]>
Co-Authored-By: Muhammad Hassan <[email protected]>
Co-Authored-By: Muhammad Hassan <[email protected]>
|
Hi @sabman, Thank you for this contribution. This feature is vital for us, so having this PR to work with is excellent! We'll review it and lat you know if anything needs to be added before merging it into srai. |
…ving-loading' into 457-geovex-model-saving-loading
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
==========================================
+ Coverage 89.74% 90.21% +0.46%
==========================================
Files 62 62
Lines 2439 2474 +35
==========================================
+ Hits 2189 2232 +43
+ Misses 250 242 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you @sabman for this contribution! I have left one change request and asked one question. Other than that it looks great.
We will also need to fill the CHANGELOG.md file, can I fill it for you, or do you want to do it yourself?
srai/embedders/geovex/embedder.py
Outdated
| # save model and config | ||
| self._model.save(path / "model.pt") # type: ignore | ||
| # combine model config and embedder config | ||
| if self._model is not None: | ||
| model_config = self._model.get_config() |
There was a problem hiding this comment.
Why is there a check for a model not being None? If self._model was None, the save function would throw an exception.
Should this be a check for a model config, or should the save function be moved inside this if statement?
There was a problem hiding this comment.
I can see in the Hex2Vec implementation that there is no such check with if statement.
There was a problem hiding this comment.
@RaczeQ thanks for the feedback. The type linter was complaining about _model being None so we put in a check. But let us remove it and push again.
srai/embedders/geovex/model.py
Outdated
| def get_config(self) -> dict[str, int | float]: | ||
| """ | ||
| Get the model configuration. | ||
|
|
||
| Returns: | ||
| Dict[str, int | float]: The model configuration. | ||
| """ |
There was a problem hiding this comment.
We are still supporting the Python 3.9 version, so it doesn't accept the pipe syntax yet and tests on this Python version failed. Please change to the Union version. It will also require additional import at the top from typing.
| def get_config(self) -> dict[str, int | float]: | |
| """ | |
| Get the model configuration. | |
| Returns: | |
| Dict[str, int | float]: The model configuration. | |
| """ | |
| def get_config(self) -> dict[str, Union[int, float]]: | |
| """ | |
| Get the model configuration. | |
| Returns: | |
| Dict[str, Union[int, float]]: The model configuration. | |
| """ |
Co-Authored-By: Muhammad Hassan <[email protected]>
|
FYI @sabman @mhassanch changes have been released with version 0.7.6 |
work for #457
This PR adds:
It also adds a test to save and load
GeoVexEmbedder. We have been running it via: but hopefully CI runs the full suite.Our first PR to this awesome project, apologies in advance for any oversights but happy to learn best practices from feedback and contribute more.
Co-authored with @mhassanch