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

Implementation of a first unit test (DL1) #34

Merged
merged 11 commits into from
Jan 21, 2020

Conversation

HealthyPear
Copy link
Member

This PR depends on PR #33

This test runs write_dl1.py using an improved version of the example analysis configuration and the whole ctapipe test file gamma_test_large.simtel.gz (110 events from a Paranal-like site and 3 cameras).

It checks if the write_dl1.py script ends successfully and as a result, it creates an HDF5 file containing events from LSTCam, FlashCam, and ASTRICam.

Caveat: for now, this test doesn't check the production of the image.h5 file containing the simulated and calibrated images (even though that is at the basis of all the operations already performed to produce successfully the DL1 file).

@kosack
Copy link
Contributor

kosack commented Nov 28, 2019

The problem seems to just be that protopipe.scripts is not a python module, so it doesn't get included in the package when it is installed.

I verified it like this:

In [1]: from setuptools import find_packages

In [2]: find_packages()
Out[2]: ['protopipe', 'protopipe.pipeline', 'protopipe.mva', 'protopipe.perf']

In setup.py you call find_packages(), so it only registers the ones listed above, not scripts, since there is no __init__.py in that directory

@HealthyPear
Copy link
Member Author

There is not a problem with adding protopipe.scripts as you say, it makes sense since all the other modules are included already by default.

Just for me to understand: did you check this locally on your machine?
Because for me locally calling from protopipe.scripts import write_dl1 works with an empty find_packages() list. Is this related to how Travis installs and runs packages?

@HealthyPear
Copy link
Member Author

Adding the init.py in protopipe.scripts solved that error, but now it cannot find the example configuration file. I thought that finding the source code folder after installation by doing os.path.dirname(protopipe.__path__[0]) was sufficient, but it seems not.

From the Travis log I can see that it's trying to find it in [...]site-packages/protopipe-0.2.1.dev0-py3.7.egg/aux/[...], but between protopipe-0.2.1.dev0-py3.7.egg and aux there should be a protopipe. In my local setup when I do that command I obtain as expected /Users/michele/Applications/ctasoft/protopipe and I add the rest of the path.

@kosack
Copy link
Contributor

kosack commented Dec 5, 2019

I think you need to

  1. make sure the example config file becomes part of the package data (I think you have to add it somewhere in setup.py or use manifest.in - see the ctapipe-extra package), otherwise it will not be stored in the package, so the tests can't use it.
  2. use the pkg_resources.resource_filename("conf.yaml") to get the file's true pathname

By the way, rather than using pytest for this, you can also just simply run the test script in the travis.yml. It will still give an error if it fails, just like in pytest. It could be easier, since this is more complex than a simple unit test. However, it would be nice to also have unit tests run by pytest.

@HealthyPear
Copy link
Member Author

I think you need to

1. make sure the example config file  becomes part of the package data (I think you have to add it somewhere in setup.py or use manifest.in - see the ctapipe-extra package), otherwise it will not be stored in the package, so the tests can't use it.

From what I could see I think adding something like,

package_data={ 'aux': ['example_config_files/protopipe/*.yml'] }

to the arguments of setup() in setup.py should be sufficient. Correct?

2. use the `pkg_resources.resource_filename("conf.yaml")`  to get the file's true pathname

This should work only after I did the previous operation, am I right?

By the way, rather than using pytest for this, you can also just simply run the test script in the travis.yml. It will still give an error if it fails, just like in pytest. It could be easier since this is more complex than a simple unit test. However, it would be nice to also have unit tests run by pytest.

For one it is maybe true, but many more will come later on.
For now - at least locally - launching pytest on this first test takes only a few seconds.

kosack
kosack previously approved these changes Jan 14, 2020
@kosack
Copy link
Contributor

kosack commented Jan 14, 2020

Seems you still get a bad exit status from the command you run:

>       assert exit_status == 0
E       assert 256 == 0

That means the command is exiting with status 256 (which is I think actually a return value of -1 from the main function in your script, probably)

@HealthyPear
Copy link
Member Author

To summarize the situation: with the help of Karl I managed to modify things in a way that the configuration file needed for the test will be always found depending only on the internal structure of the package.

Indeed, now the Travis CI seems to not fail because it doesn't find the configuration file (on its remote machine) but because it doesn't know the name of a camera.
This should be solved when PR #33 gets approved, merged into master and from there merged here.

@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@7a3dc40). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master     #34   +/-   ##
========================================
  Coverage          ?   0.43%           
========================================
  Files             ?      20           
  Lines             ?    2084           
  Branches          ?       0           
========================================
  Hits              ?       9           
  Misses            ?    2075           
  Partials          ?       0
Impacted Files Coverage Δ
protopipe/scripts/write_dl1.py 0% <0%> (ø)
protopipe/scripts/write_dl2.py 0% <0%> (ø)
protopipe/pipeline/utils.py 0% <0%> (ø)
protopipe/pipeline/event_preparer.py 0% <0%> (ø)
protopipe/pipeline/image_cleaning.py 0% <0%> (ø)
protopipe/perf/irf_maker.py 0% <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 7a3dc40...ffa6bd5. Read the comment docs.

@HealthyPear HealthyPear merged commit 27d8827 into cta-observatory:master Jan 21, 2020
@HealthyPear HealthyPear deleted the feature-unit_tests branch January 21, 2020 15:23
@HealthyPear HealthyPear added maintenance and removed enhancement New feature or request labels Oct 23, 2020
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