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

to_dict() and from_dict() functionality for Coregionalize Kernel and MixedNoise Likelihood class #951

Merged
merged 31 commits into from
Dec 9, 2021

Conversation

gehbiszumeis
Copy link
Contributor

@gehbiszumeis gehbiszumeis commented Oct 29, 2021

These are mandatory prerequites for tracking GPy.models.GPCoregionalizedRegression models with Machine learning lifecycle tools (e.g mlflow)

@gehbiszumeis gehbiszumeis changed the title to_dict() and from_dict() functionality for Coregionalize Kernel and and MixedNoise Likelihood class to_dict() and from_dict() functionality for Coregionalize Kernel and MixedNoise Likelihood class Oct 29, 2021
@ekalosak
Copy link
Contributor

ekalosak commented Nov 4, 2021

Looks like a CI error, otherwise LGTM. Any thoughts on the CI error?
Also, where're the from_dict implementations? Or, are they inherited?

@gehbiszumeis
Copy link
Contributor Author

Looks like a CI error, otherwise LGTM. Any thoughts on the CI error?

Yes, according to AppVeyor it seems, that it fails already doing the build for almost all environments. It cannot install a package apparently

ERROR conda.core.link:_execute(502): An error occurred while installing package 'defaults::charset-normalizer-2.0.4-pyhd3eb1b0_0'.

No idea why, I didn't touch the requierements.txt. Only for the py3.5 environment it runs past this step and breaks when running the code coverage test

GPy: Saving user configuration file to C:\Users\appveyor\.config\GPy\user.cfg coverage run travis_tests.py ........S............S...............................................................................................................................................................................................................................................................................................................................S.............................................................................................Command exited with code -1073741819

Also, where're the from_dict implementations? Or, are they inherited?

Yes, the corresponding from_dict() methods of the super classes are used. I added only the _build_from_input_dict methods to add some specific parameters to the corresponding input_dict.

@ekalosak
Copy link
Contributor

Fun enough, the errors are different across Python versions.

Python 3.5

Screen Shot 2021-11-10 at 1 14 52 PM

## Python 3.6 thru 3.9

Screen Shot 2021-11-10 at 1 16 56 PM

## Things to try - The `conda clean` command in the 3.6 error message - Looks like the 3.5 error code shows up [here](https://github.com/TryGhost/node-sqlite3/issues/1318) as well, something to think about...

@gehbiszumeis
Copy link
Contributor Author

Things to try - The conda clean command in the 3.6 error message - Looks like the 3.5 error code shows up here as well, something to think about...

The 3.5 error disappeared, the >3.6 errors persist. I tried quite some stuff manipulating the way conda is trying to install packages, with no success yet. After all the trying it seems, that the charset-normalizer package has only a noarch version and no win64 version.

What really puzzles me is that the build fails only for this PR and not for others, so it cannot be some general bug in charset-normalizer or similar. It seems that for my build it tries to install that package, while for other PR builds it doesn't. That's weird

ppk42 and others added 5 commits November 25, 2021 18:49
I also use a newer miniconda version for greater python versions.
Thinking it over it decided to use miniconda38 for all python versions unless python 3.5.
A try to get rid of the appveyor build error.
Peter Paul Kiefer added 3 commits November 27, 2021 16:03
After bringing the miniconda env to work again, the wrong matplotlib version was used. This commit should fix that.
Freezing numpy and scipy was a bad idea.
I freeze matplotlib  dependend  on the python version only.
@ppk42
Copy link
Contributor

ppk42 commented Nov 28, 2021

I brought the appveyor build back to life. The pull request is still open. But, you'll find the changes in the last commit. (a93c4ac)

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #951 (4286501) into devel (3e19a85) will decrease coverage by 0.08%.
The diff coverage is 30.30%.

@@            Coverage Diff             @@
##            devel     #951      +/-   ##
==========================================
- Coverage   54.59%   54.50%   -0.09%     
==========================================
  Files         211      210       -1     
  Lines       21564    21557       -7     
  Branches     2891     3168     +277     
==========================================
- Hits        11772    11750      -22     
- Misses       9240     9253      +13     
- Partials      552      554       +2     

@ekalosak
Copy link
Contributor

ekalosak commented Dec 9, 2021

Brilliant work both, thank you for the contributions!
Because the code coverage is still not really a priority, passing builds and tests is totally sufficient so I'm going to merge this.
@lawrennd @apaleyes <- please give this an 👁️ so we can remove it if it's bad.

@ekalosak ekalosak merged commit bb1bc50 into SheffieldML:devel Dec 9, 2021
@lawrennd
Copy link
Member

Thanks @ekalosak and @ppk42!

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.

4 participants