kiudee / cs-ranking

Context-sensitive ranking and choice in Python with PyTorch
https://cs-ranking.readthedocs.io
Apache License 2.0
66 stars 15 forks source link

Move logger initialization to the module level #154

Closed timokau closed 4 years ago

timokau commented 4 years ago

Description

This is what is usually recommended in python logging tutorials (including the official documentation). It also frees us up from having the "logging" attribute in our estimators, which the scikit-learn check doesn't like (although it doesn't really hurt in this case, since a logger hardly counts as state).

Motivation and Context

Following standard practices and making progress on the "no_attributes_set_in_init" check of the scikit-learn estimator check suite.

How Has This Been Tested?

Testsuite and pre-commit.

Does this close/impact existing issues?

Impacts #94, #116.

Types of changes

Checklist:

codecov[bot] commented 4 years ago

Codecov Report

Merging #154 into master will increase coverage by 0.46%. The diff coverage is 62.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   56.57%   57.04%   +0.46%     
==========================================
  Files         113      113              
  Lines        6543     6560      +17     
==========================================
+ Hits         3702     3742      +40     
+ Misses       2841     2818      -23     
Impacted Files Coverage Δ
csrank/dataset_reader/tag_genome_reader.py 15.20% <5.88%> (+0.46%) :arrow_up:
...nk/dataset_reader/letor_listwise_dataset_reader.py 14.56% <7.14%> (+0.48%) :arrow_up:
...ank/dataset_reader/letor_ranking_dataset_reader.py 14.52% <7.14%> (+0.55%) :arrow_up:
csrank/dataset_reader/expedia_dataset_reader.py 19.71% <11.11%> (+0.70%) :arrow_up:
...taset_reader/objectranking/image_dataset_reader.py 20.27% <14.28%> (+0.69%) :arrow_up:
csrank/discretechoice/model_selector.py 16.43% <14.28%> (+1.36%) :arrow_up:
...retechoice/mnist_discrete_choice_dataset_reader.py 13.13% <25.00%> (+1.79%) :arrow_up:
csrank/dataset_reader/mnist_dataset_reader.py 30.00% <25.00%> (+2.00%) :arrow_up:
...r/objectranking/neural_sentence_ordering_reader.py 27.27% <25.00%> (+1.51%) :arrow_up:
...rank/dataset_reader/synthetic_dataset_generator.py 31.81% <25.00%> (+1.51%) :arrow_up:
... and 58 more
timokau commented 4 years ago

@kiudee the travis failures here are very confusing to me. I think they are due to a cached python38 miniconda env. Could you clear the travis caches?

kiudee commented 4 years ago

I removed all caches and restarted the builds. I will soon setup a pyproject.toml, which should help us clean up all that dependency setup on travis.

timokau commented 4 years ago

Travis is still unhappy for some reason (one more thread on it: https://travis-ci.community/t/expected-waiting-for-status-to-be-reported-is-back/7550/7). Maybe #156 will help, otherwise we might just move to GH actions. Either way, probably safe to ignore here.

Edit: Just to be clear, clearing the caches works. The build is no longer failing. But there is still the "Expected — Waiting for status to be reported" issue.

kiudee commented 4 years ago

I think the reason is that I can only restart the build of the pull request in the travis-ci dashboard. Can you try triggering a rebuild of the branch using:

git commit --allow-empty -m "Trigger"
timokau commented 4 years ago

I don't think that is it, I have seen this issue multiple times already when trying to work around the caches issue. I'll try again though.

timokau commented 4 years ago

So this just resurrected the other error again :smile: Do you plan to finish #110 near term? Or should I look into it? Its probably better to focus on that instead of patching up the current solution.

kiudee commented 4 years ago

Yes, I made some progress on the travis-tox integration in conjunction with our parallel tests. I will push a first version for discussion soon.

timokau commented 4 years ago

In that case I propose to ignore the travis issue for now. I have removed the trigger commit again.