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

Implement custom MeanLayer in nn_ensemble #500

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

osma
Copy link
Member

@osma osma commented Jun 16, 2021

This PR replaces the Lambda layer with a custom MeanLayer in the nn_ensemble backend. It was intended as a fix to the CustomMaskWarning error encountered in PR #499 (upgrade to TensorFlow 2.5.0) but it turned out that the problem is unrelated.

This might still be a good idea, for example it could improve the compatibility of saved nn_ensemble models between Python versions. But it needs more testing so I'm leaving this as a draft PR.

@osma osma added this to the Short term milestone Jun 16, 2021
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #500 (2403423) into master (0fe6557) will increase coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files          78       78           
  Lines        5669     5672    +3     
=======================================
+ Hits         5640     5643    +3     
  Misses         29       29           
Impacted Files Coverage Δ
annif/backend/nn_ensemble.py 99.23% <83.33%> (-0.77%) ⬇️
annif/backend/stwfsa.py 100.00% <0.00%> (+1.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fe6557...2403423. Read the comment docs.

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma osma mentioned this pull request Jun 16, 2021
@osma
Copy link
Member Author

osma commented Aug 4, 2021

I tested this in a situation where Python 3.6 is used for training models and Python 3.8 for using them. The PR seems to improve compatibility between Python versions for NN ensemble projects, but both instances need to run the updated code.

First I shamelessly copied the minimal setup from #453:

# projects.cfg:
[arch-fasttext]
name=FastText
language=fi
backend=fasttext
limit=100
vocab=arch
analyzer=snowball(finnish)

[arch-nn]
name=YSO NN ensemble Finnish
language=fi
backend=nn_ensemble
sources=arch-fasttext
limit=100
vocab=arch
nodes=100
dropout_rate=0.2
epochs=2

# Training on Python 3.6:
annif loadvoc arch-fasttext tests/corpora/archaeology/subjects.tsv
annif train arch-fasttext tests/corpora/archaeology/documents.tsv
annif train arch-nn tests/corpora/archaeology/fulltext/

# Testing on Python 3.8:
echo arkeologia | annif suggest arch-nn

Without this PR, the annif suggest command fails with this error:

ValueError: bad marshal data (unknown type code)

With this PR applied, it works without problems.

I also tested the case of old models trained without this PR. They still keep working with the PR applied. So this PR doesn't break compatibility for old NN ensemble projects, but projects will have to be retrained to be able to benefit from the additional compatibility this PR offers.

@osma osma marked this pull request as ready for review August 4, 2021 07:06
@osma osma requested a review from juhoinkinen August 4, 2021 07:07
@juhoinkinen
Copy link
Member

Like above I tested training models on Python 3.7 and using them on 3.9: without this PR the bad marshal data (unknown type code) error appears, but with this PR applied it does not (suggest call works).

@osma osma merged commit 483f047 into master Aug 12, 2021
@osma osma deleted the feature-nn-ensemble-meanlayer branch August 12, 2021 06:35
@juhoinkinen juhoinkinen modified the milestones: Short term, 0.54 Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants