Matplotlib drawer: fix bug #1437 and add tests

[sergiomtzlosa]

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[alecandido]

Already noticed that the CI is still not running...

[scarrazza]

@alecandido I have the suspicion that they changed on tag name to pull_request_target: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target

[alecandido]

@scarrazza this is the only related change I've found in the last six months

github/docs@9e3e157

But maybe I should keep checking, I may have missed something.
The file is the following:
https://github.com/github/docs/blob/main/content/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows.md

(I believe the workflow runner is not even open source, but GitHub docs are, so we can check those, at least)

[scarrazza]

Ok, the problem is the if condition in rules.yml:

qibo/.github/workflows/rules.yml

Line 13 in 4f2314a

if: contains(github.event.pull_request.labels.*.name, 'run-workflow') || github.event_name == 'push'

This line is forcing a skip due to missing pushes (this PR was opened with a series of pushes already performed).
We should add the "PR opened event" in the if condition too.

[alecandido]

Correct, thanks for the investigation.

In any case, even the run-workflow label is definitely a good solution. For sure for this PR.

[MatteoRobbiati]

Thanks a lot @scarrazza for spotting the CI problem and @sergiomtzlosa for the fix.
The lint is now fine. Anyway, there is a problem in the test code we added to the documentation. Please remove the %matplotlib inline line from this example and ask me for a new review @sergiomtzlosa :)

[sergiomtzlosa]

thanks @alecandido @scarrazza for tune in the CI pipeline, I removed the line!!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (218f298) to head (51cd75d).
Report is 127 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
+ Coverage   97.10%   99.94%   +2.83%     
==========================================
  Files          81       81              
  Lines       11653    11708      +55     
==========================================
+ Hits        11316    11701     +385     
+ Misses        337        7     -330     

Flag	Coverage Δ
unittests	99.94% <100.00%> (+2.83%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MatteoRobbiati]

Thanks @sergiomtzlosa for the fix!
Now we miss the tests. I would suggest to create a test_ui_mlpdrawings.py file (or something like that) in which we test all the code of your contribution.
Some dummy tests will be enough to cover it.
A complete report of the covered lines is provided by codecov everytime a new commit is pushed here (see last lines in the checks list) :)

Let me know if you need some help in writing/defining the tests!

[sergiomtzlosa]

Hi, thanks @MatteoRobbiati , just a question, which version should I install to do the test? I had installed pytest and I get this errors on runing the tests:

> pytest --version
pytest 8.3.2
__main__.py: error: unrecognized arguments: --cov=qibo --cov-append --cov-report=xml --cov-report=html
  inifile: ..\qibo\pyproject.toml
  rootdir: ..\qibo`
> pytest test_ui_mpldrawer.py
ERROR: usage: __main__.py [options] [file_or_dir] [file_or_dir] [...]
__main__.py: error: unrecognized arguments: --cov=qibo --cov-append --cov-report=xml --cov-report=html
  inifile: ..\qibo\pyproject.toml
  rootdir: ..\qibo`

Maybe codecov is not triggering....

[MatteoRobbiati]

During the meeting we agreed in setting up a bit more robust strategy to test the images. What is done so far is perfectly fine, but we just need to be sure the output of our plots is actually what we expect during the tests. For doing this, we can:

1. generate locally the images we are re-generating in the tests;
2. convert them into Numpy arrays and store those arrays in a folder;
3. during tests, check whether the new plots, converted into arrays, are actually equal to the target arrays.

Alternatively, we can also think to use the Matplotlib testing tools.

[sergiomtzlosa]

During the meeting we agreed in setting up a bit more robust strategy to test the images. What is done so far is perfectly fine, but we just need to be sure the output of our plots is actually what we expect during the tests. For doing this, we can:

1. generate locally the images we are re-generating in the tests;

2. convert them into Numpy arrays and store those arrays in a folder;

3. during tests, check whether the new plots, converted into arrays, are actually equal to the target arrays.

Alternatively, we can also think to use the Matplotlib testing tools.

Good!!! I will start with the plot arrays comparison...

[MatteoRobbiati]

Good!!! I will start with the plot arrays comparison...

Hi there! Do you need any help here? It would be ideal to fix this for the next review (friday). Let me know about the status/if help is needed @sergiomtzlosa.

[sergiomtzlosa]

Good!!! I will start with the plot arrays comparison...

Hi there! Do you need any help here? It would be ideal to fix this for the next review (friday). Let me know about the status/if help is needed @sergiomtzlosa.

Hi @MatteoRobbiati , everything is done in the latest commit, please let me know if something is missing, you can launch CI indeed

[sergiomtzlosa]

Hi @MatteoRobbiati , I am wondering why all test are correct , but the macos test fail or got cancelled, please can you gibe some light? Is the CI maybe crashing?

[MatteoRobbiati]

Thanks for this @sergiomtzlosa! It sounds like what I had in mind. I was thinking if the PNG images are the best way to store the data, but I think this can work.

1. PIL is used to test the assertions here, but I think it has to be added to the pyproject.toml and update the lock (we need to specify this package has to be installed when performing tests);
2. tests are failing on ios right now. It seems the arrays are not equivalent.. maybe we should consider to use some specific Matplotlib backend?

[MatteoRobbiati]

Hi @MatteoRobbiati , I am wondering why all test are correct , but the macos test fail or got cancelled, please can you gibe some light? Is the CI maybe crashing?

Sorry!! Missed it! Remember also to ask me for review once you are done with modifications :)

[sergiomtzlosa]

Hi @MatteoRobbiati , I am wondering why all test are correct , but the macos test fail or got cancelled, please can you gibe some light? Is the CI maybe crashing?

Sorry!! Missed it! Remember also to ask me for review once you are done with modifications :)

Hi @MatteoRobbiati , now I have a big issue, I cannot fix this until the next week then I cannot finish this for friday, and i would ask for some help to get this on time otherwise i fix it next week, sorry!

[alecandido]

Testing exact equality pixel by pixel is definitely a hard constraint, so you should make sure there is no platform-dependent behavior in matplotlib as well (even driven by the environment, i.e. variables and packages available on a certain system).

A different option could be generating the pictures directly in the CI, and caching them somewhere, separately for each platform, skipping the tests if they are not available.
However, while this option will be more robust from the point of view of platform-dependent behaviors, but it will be much more expensive (time-wise) to instrument...

If you're able to debug the issue, and fix some env var to obtain the same result, so much the better.
Otherwise, I'd consider acceptable even just testing that an image file is produced, whatever the content is, since it will tell us that the code didn't raise/crash (at least).

[sergiomtzlosa]

Finally, I could take some time and I changed the images by arrays in files for matching, so it is not needed to use PIL library, also I set the matplotlib backend and I hope this time all test are passing, otherwise I will go with the solutions you suggested @alecandido @MatteoRobbiati

[MatteoRobbiati]

Thanks a lot for this effort @sergiomtzlosa, and again sorry not to have noticed this before.
Now everything sounds good to me. I re run the tests and everything is passing.

[sergiomtzlosa]

Thanks @alecandido @MatteoRobbiati , finally we made It!