Do continuous integration and codecoverage as in reduceSPI #19

geoffwoollard · 2021-10-05T00:16:01Z

Some notes on what I did that deserve a closer look

Badges in README.md. What am I supposed to use for the links at the top of https://raw.githubusercontent.com/compSPI/reduceSPI/master/README.md
reduceSPI just has tests in tests/test_*.py. However ioSPI has tests/test_*.py and also tests/test_iotools/test_*.py. What should be in ioSPI/ci_codecov/pytest.ini to handle the extra folder? geoffwoollard@b668095
I took the isort and black transformers out of .deepsource.toml geoffwoollard@d99cc15
.codecov.yml has an extra part to ignore folders that are absent in reduceSPI. I removed coverage folder because its empty in ioSPI and absent in reduceSPI. geoffwoollard@1980c1e
.github/workflows/build.yml is quite a bit different. Note that the flags for black are absent. geoffwoollard@6311a51#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721L51 These were important to have the linting and testing pass.
Aside: there's a folder iospi/iotools and also ioSPI/iotools/. They have different .py files in them. Is it supposed to be like that?

…and Build not copy pasted, and DeepSource alredy present

…eSPI

…mpty in ioSPI and absent in reduceSPI

…version of pip (warning in continuous integration)

…ne should end with a period

…ll, but did not result in any comittable changes

geoffwoollard · 2021-10-05T01:17:26Z

I'm struggling to satisfy isort/flake8/black at the same time. what is the workflow? I'm isorting, but then flake8 has an issue

(ioSPI) dhcp-128-189-219-129:ioSPI gw$ isort /Users/gw/repos/ioSPI/tests/test_fourier.py
(ioSPI) dhcp-128-189-219-129:ioSPI gw$ git status
On branch ci_codecov
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   tests/test_fourier.py

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   tests/test_fourier.py

(ioSPI) dhcp-128-189-219-129:ioSPI gw$ git commit tests/test_fourier.py -m 'isorted'
Check for byte-order marker..............................................Passed
Check for case conflicts.................................................Passed
Check for merge conflicts................................................Passed
Check Yaml...........................................(no files to check)Skipped
Mixed line ending........................................................Passed
Don't commit to branch...................................................Passed
Check for added large files..............................................Passed
Trim Trailing Whitespace.................................................Passed
black....................................................................Passed
isort....................................................................Passed
blacken-docs.............................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/test_fourier.py:1:1: D100 Missing docstring in public module
tests/test_fourier.py:3:1: I202 Additional newline in a group of imports. 'from ioSPI import fourier' is identified as Third Party and 'import numpy' is identified as Third Party.

(ioSPI) dhcp-128-189-219-129:ioSPI gw$

geoffwoollard · 2021-10-05T01:24:54Z

Is this a possible fix?
PyCQA/flake8-import-order#145
https://github.com/PyCQA/flake8-import-order#configuration

ninamiolane · 2021-10-05T02:52:28Z

@geoffwoollard the pre-commit hook flake8 does not correct the files automatically, it is just failing. In this case, we need to write the error messages and address them. Here:

there is a docstring missing at the very top of the test_fourier.py file (isort cannot write a docstring on its own)
the second one is a bit weirder, it seems to say that you need an additional blank line before this ordeR? Did you try this?

Badges in README.md. What am I supposed to use for the links at the top of https://raw.githubusercontent.com/compSPI/reduceSPI/master/README.md

Does it work to just replace reduceSPI by ioSPI?

reduceSPI just has tests in tests/test_.py. However ioSPI has tests/test_.py and also tests/test_iotools/test_*.py. What should be in ioSPI/ci_codecov/pytest.ini to handle the extra folder? geoffwoollard/ioSPI@b668095

Let's use tests/test_*.py for now. We'll organize them in subfolders when we have more tests: bottom-up approach = we do it only when we need it.

I took the isort and black transformers out of .deepsource.toml geoffwoollard/ioSPI@d99cc15

Yay thanks.

.codecov.yml has an extra part to ignore folders that are absent in reduceSPI. I removed coverage folder because its empty in ioSPI and absent in reduceSPI. geoffwoollard/ioSPI@1980c1e

Cool. thanks.

.github/workflows/build.yml is quite a bit different. Note that the flags for black are absent. geoffwoollard/ioSPI@6311a51#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721L51 These were important to have the linting and testing pass.

OK!

Aside: there's a folder iospi/iotools and also ioSPI/iotools/. They have different .py files in them. Is it supposed to be like that?

No, it's not meant to be like this: ioSPI/iotools is redundant, since we know that we will have iotools in ioSPI. Let's use the bottom up approach again, and only have one ioSPI folder, with *.py modules in it. We'll subdivide this in subfolders when we need to (not before!).

geoffwoollard · 2021-10-05T03:35:47Z

The problem is that flake8 wants

import numpy as np
from ioSPI import fourier

And isort wants

import numpy as np

from ioSPI import fourier

When I commit with the hooks, isort changes things to

import numpy as np

from ioSPI import fourier

Then flake8 fails.

These links say I need to tell flake8 somehow that there should be a line between them, because ioSPI is local, not third party.
PyCQA/flake8-import-order#145
https://github.com/PyCQA/flake8-import-order#configuration

…tatement, otherwise tries to put together. But isort knows it is local so automatically edits

geoffwoollard · 2021-10-05T03:37:04Z

Actually this works

import numpy as np

from ..ioSPI import fourier

…ith capitals. two folders

geoffwoollard · 2021-10-05T04:07:32Z

There is an issue with iospi and ioSPI. When I checked out the files inside iospi from the main branch, then went inside ioSPI. So now on my branch there is no iospi, only ioSPI folder. Hopefully it stays that way when I merge and there is only only the ioSPI folder and no more iospi.

…hree dots

ninamiolane · 2021-10-05T04:16:43Z

.flake8

+docstring-convention = numpy
+import_order_style = smarkets
+max-line-length = 88
+extend-ignore = E203


@geoffwoollard how about adding I202 to the extend-ignore line?

…ts own __init__.py file and it can not go above that level to ioSPI

…are in the wrong order. 'import ioSPI.iotools.cryo_dataset' should be before 'import torch'

…hange back name after pass test

geoffwoollard · 2021-10-05T05:23:17Z

I was really confused for a while that the CI couldn't find this module. Locally it was in ioSPI, but actually git saw it as in iospi. So I just copied it (cp, not git mv) and renamed it as cryodataset.py. 2b78842

(ioSPI) dhcp-128-189-219-129:iotools gw$ git mv cryo_dataset.py cryodataset.py
fatal: not under version control, source=ioSPI/iotools/cryo_dataset.py, destination=ioSPI/iotools/cryodataset.py

geoffwoollard · 2021-10-05T05:26:08Z

@fredericpoitevin maybe you can write some tests for atomic_models.py?

----------- coverage: platform linux, python 3.9.5-final-0 -----------
Name                             Stmts   Miss  Cover   Missing
--------------------------------------------------------------
ioSPI/__init__.py                    1      0   100%
ioSPI/fourier.py                    67     17    75%   26-33, 54-61, 104
ioSPI/iotools/__init__.py            0      0   100%
ioSPI/iotools/atomic_models.py      51     35    31%   45, 47, 54, 76-80, 102-110, 127-134, 156-168
ioSPI/iotools/cryodataset.py        71     12    83%   36, 38-39, 47-50, 56-59, 87
--------------------------------------------------------------
TOTAL                              190     64    66%
Coverage XML written to file coverage.xml

FAIL Required test coverage of 90.0% not reached. Total coverage: 66.32%

============================= 15 passed in 12.03s ==============================
Error: Process completed with exit code 1.

codecov · 2021-10-05T05:33:07Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cf53c24). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 54ad298 differs from pull request most recent head a34d93f. Consider uploading reports for the commit a34d93f to get more accurate results

@@            Coverage Diff            @@
##             master      #19   +/-   ##
=========================================
  Coverage          ?   66.32%           
=========================================
  Files             ?        4           
  Lines             ?      190           
  Branches          ?        0           
=========================================
  Hits              ?      126           
  Misses            ?       64           
  Partials          ?        0

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 cf53c24...a34d93f. Read the comment docs.

ninamiolane

Looking good! We can merge this since most of it is working, and it will give us a chance to see if everything else is in place at the next PR.

But only if you can tackle the 90% code coverage threshold ASAP?

ninamiolane · 2021-10-05T14:18:32Z

.codecov.yml

    project:
      default:
-        target: 90%
+        target: 50%


This should be 90% though: this is the threshold under which the PR will not pass: we don't want a PR to pass if we don't have a least 90% of the code that is covered, i.e. tested by the unit-tests. Why the decrease here?

Ok I'll see if I can write some tests for Fred's atomic_models.py

ninamiolane · 2021-10-05T14:18:39Z

.codecov.yml

      default:
        # basic
-        target: 90%
+        target: 50%


ninamiolane · 2021-10-05T14:18:56Z

.codecov.yml

 ignore:
  - "data"
-  - "coverage"
+  - "notebooks"


We will have to test the notebooks actually, but we can leave this for later I guess.

ninamiolane · 2021-10-05T14:21:05Z

.flake8

+docstring-convention = numpy
+import_order_style = smarkets
+max-line-length = 88
+extend-ignore = E203,I202,I100


Just curious, what is I100? Do you know why I didn't need these exclude in reduceSPI?

It's very similar as I202, with the spacing between the imports. Flake does not know that ioSPI is local, and wants to put the imports together, while isort knows and splits them up. See 2d7e03c

In your tests you do https://github.com/compSPI/reduceSPI/blob/master/tests/test_offline.py#L3, but there is no other import like import numpy or something nearby. My guess is that you would have the same issue.

… tests

geoffwoollard · 2021-10-05T15:55:36Z

I wrote some tests, but the CI is not happening? Where did they go?

ninamiolane · 2021-10-05T15:58:41Z

Oh yeah, I had the same problem with reduceSPI at some point... and then it came back! I don't know what is happening, TBH it could be that github action is experiencing a bug, or latency?

Maybe let's try to merge this, and see what it gives on a new PR?

ninamiolane · 2021-10-05T15:59:03Z

Oh lol, it's back x)

geoffwoollard · 2021-10-05T20:58:36Z

There's some problem with the code coverage. I wrote tests for parts that is says aren't covered. They are special because they are @Jitted, so maybe there's some issue with that.

----------- coverage: platform linux, python 3.9.5-final-0 -----------
Name                             Stmts   Miss  Cover   Missing
--------------------------------------------------------------
ioSPI/__init__.py                    1      0   100%
ioSPI/fourier.py                    67     16    76%   26-33, 54-61
ioSPI/iotools/__init__.py            0      0   100%
ioSPI/iotools/atomic_models.py      51      1    98%   159
ioSPI/iotools/cryodataset.py        71     12    83%   36, 38-39, 47-50, 56-59, 87
--------------------------------------------------------------
TOTAL                              190     29    85%

The tests are https://github.com/geoffwoollard/ioSPI/blob/ci_codecov/tests/test_fourier.py#L51 and https://github.com/geoffwoollard/ioSPI/blob/ci_codecov/tests/test_fourier.py#L65

ninamiolane · 2021-10-05T21:52:40Z

Let's merge and see what codecov says. Thanks for this 🎉

geoffwoollard added 11 commits October 4, 2021 16:34

copy pasted and modified from reduceSPI

6311a51

copy pasted and modified from reduceSPI

5ee678a

copy pasted and modified from reduceSPI

4d3a95f

copy pasted and modified from reduceSPI

c8cf44b

copy pasted and modified from reduceSPI

d99cc15

copy pasted and modified from reduceSPI. links to badges for Codecov …

97aca80

…and Build not copy pasted, and DeepSource alredy present

renamed to environment.yml

088c505

will need to add path to tests/test_iotools

b668095

add in notebooks folder.

2417a9e

remove coverage folder because its empty in ioSPI and absent in reduc…

419d4ec

…eSPI

notebook folder added in before. remove coverage folder because its e…

1980c1e

…mpty in ioSPI and absent in reduceSPI

geoffwoollard requested review from fredericpoitevin and ninamiolane October 5, 2021 00:16

geoffwoollard added 3 commits October 4, 2021 17:18

include pip explicitly and by itself in environment.yml to use right …

54ea5f1

…version of pip (warning in continuous integration)

ci error tests/test_iotools/test_atomic_models.py:14:1: D400 First li…

f7dcebf

…ne should end with a period

forgot .flake8 and .pre-commit-config.yaml. also ran pre-commit insta…

7c8f441

…ll, but did not result in any comittable changes

flake8 needs to know that ioSPI is local with two dots .. in import s…

7ea2266

…tatement, otherwise tries to put together. But isort knows it is local so automatically edits

geoffwoollard added 5 commits October 4, 2021 20:40

add two dots to import

aff5b6c

docstrings

ad00575

First line should be in imperative mood

7308f59

add tests/test_iotools/__init__.py to .flake8 exclude

4085f7c

iospi datasets in ioSPI. checkout from master. note issue on github w…

da321a0

…ith capitals. two folders

relative level of the package to import is from the file itself, so t…

e94fcf0

…hree dots

ninamiolane reviewed Oct 5, 2021

View reviewed changes

geoffwoollard added 4 commits October 4, 2021 21:23

sorting out import .. and flake8 I202

dec1807

no more . imports because does not work in testing because that has i…

7e52604

…ts own __init__.py file and it can not go above that level to ioSPI

avoid erorr because flake does not know local: 100 Import statements …

2d7e03c

…are in the wrong order. 'import ioSPI.iotools.cryo_dataset' should be before 'import torch'

issues wtih lower case folder and uppercase folder. just rename and c…

2b78842

…hange back name after pass test

relax to 50% coverage to see if passes everything else

35a328a

change 90% coverage to 50%

7ab57a5

geoffwoollard added 2 commits October 4, 2021 22:37

made thredhold in test more lax because failed

a59484f

replaced reduce -> io. maybe Codecov link needs new token?

54ad298

ninamiolane reviewed Oct 5, 2021

View reviewed changes

geoffwoollard added 6 commits October 5, 2021 08:21

Max Perutz's hemoglobin structure from the 1970s. classic example for…

2d35d86

… tests

pdb test in atomo_models

b095126

90% coverage

20466d3

test_read_gemmi_model_cif

8dc3c92

test_write_gemmi_model_pdb test_write_gemmi_model_cif

65c2132

float. check ci working

94651f1

float back to int. ci not working

c873a06

geoffwoollard added 3 commits October 5, 2021 09:05

moved data to tests. maybe put in data folder later

5036b29

get pdb/cif file from os.system

3255866

use wget inside of testig functions so data is in right place

a34d93f

ninamiolane approved these changes Oct 5, 2021

View reviewed changes

ninamiolane merged commit 095e303 into compSPI:master Oct 5, 2021

geoffwoollard mentioned this pull request Nov 9, 2021

Get CI working, increase test coverage (numba jit) #21

Merged

Do continuous integration and codecoverage as in reduceSPI #19

Do continuous integration and codecoverage as in reduceSPI #19

Uh oh!

Conversation

geoffwoollard commented Oct 5, 2021

Uh oh!

geoffwoollard commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffwoollard commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ninamiolane commented Oct 5, 2021

Uh oh!

geoffwoollard commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

ninamiolane Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

codecov bot commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ninamiolane left a comment

Choose a reason for hiding this comment

Uh oh!

ninamiolane Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

geoffwoollard Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

ninamiolane Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

ninamiolane Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

ninamiolane Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

geoffwoollard Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

ninamiolane commented Oct 5, 2021

Uh oh!

ninamiolane commented Oct 5, 2021

Uh oh!

geoffwoollard commented Oct 5, 2021

Uh oh!

ninamiolane commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

geoffwoollard commented Oct 5, 2021 •

edited

Loading

geoffwoollard commented Oct 5, 2021 •

edited

Loading

geoffwoollard commented Oct 5, 2021 •

edited

Loading

codecov bot commented Oct 5, 2021 •

edited

Loading