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

Improve setup and change source code layout to comply with PEP-660. #32

Merged
merged 8 commits into from
Jan 25, 2023

Conversation

jlenain
Copy link
Contributor

@jlenain jlenain commented Jan 9, 2023

Following @maxnoe 's path on ctapipe_io_lst, this PR proposes to improve the setuptools configuration to comply with PEP-660.

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Base: 96.05% // Head: 87.05% // Decreases project coverage by -9.00% ⚠️

Coverage data is based on head (dec1072) compared to base (c17671c).
Patch coverage: 100.00% of modified lines in pull request are covered.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   96.05%   87.05%   -9.00%     
==========================================
  Files           8        7       -1     
  Lines         228      510     +282     
==========================================
+ Hits          219      444     +225     
- Misses          9       66      +57     
Impacted Files Coverage Δ
src/ctapipe_io_nectarcam/__init__.py 80.75% <ø> (ø)
src/ctapipe_io_nectarcam/anyarray_dtypes.py 100.00% <ø> (ø)
src/ctapipe_io_nectarcam/calibration.py 100.00% <ø> (ø)
src/ctapipe_io_nectarcam/constants.py 100.00% <ø> (ø)
src/ctapipe_io_nectarcam/containers.py 100.00% <ø> (ø)
src/ctapipe_io_nectarcam/_dev_version/__init__.py 100.00% <100.00%> (ø)
src/ctapipe_io_nectarcam/version.py 100.00% <100.00%> (ø)
...pe_io_nectarcam/tests/test_nectarcameventsource.py
src/ctapipe_io_nectarcam/tests/test_version.py
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jlenain jlenain requested a review from maxnoe January 11, 2023 22:43
@jlenain jlenain added the bug Something isn't working label Jan 11, 2023
@jlenain jlenain linked an issue Jan 11, 2023 that may be closed by this pull request
@jlenain jlenain requested review from a team and removed request for maxnoe January 24, 2023 10:35
setup.cfg Outdated
ctapipe~=0.12
numpy~=1.22
protozfits~=2.0
pytables>=3.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package name on pypi is tables, without the py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda calls it pytables though (so it is correct in environment.yml)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @maxnoe ! How mean...

@dkerszberg
Copy link

dkerszberg commented Jan 24, 2023

The change seems to work on my side. @frnbrun ca you confirm it fixed the problem you reported in cta-observatory/nectarchain#28 ?

@jlenain I see one test failed with:
FAILED src/ctapipe_io_nectarcam/tests/test_version.py::test_version - AssertionError: assert '0.0.0' != '0.0.0'
It seems to be related with your change of version (which is a good idea I agree :-) ), can we remove the test on the version or update it to something that won't fail (I didn't check at all what is done there tbh)?

@maxnoe
Copy link
Member

maxnoe commented Jan 24, 2023

@dkerszberg the test fails because there is no tag yet in this repository. Either switch it off for the time being, or create an 0.1.0 tag on the master before merging this.

@jlenain
Copy link
Contributor Author

jlenain commented Jan 24, 2023

Hi @dkerszberg,
Thanks for the feedback!
Indeed, as pointed out by @maxnoe , this test error will vanish as soon as we start tagging the code. I'd prefer to keep this mechanism in place, which is actually based on the implementation e.g. from astropy, ctapipe, or ctapipe_io_lst, since it is meant to automatize and ease release management.

@dkerszberg
Copy link

Thank you both for the clarification! I see @jlenain already clarified in the other thread that it fixes the issue from @frnbrun so I'm ready to do the merge. I'll just wait until tomorrow in case @frnbrun wants to have a look :-)

Copy link

@dkerszberg dkerszberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to be merged

@jlenain jlenain merged commit b398d39 into cta-observatory:master Jan 25, 2023
@frnbrun
Copy link

frnbrun commented Jan 25, 2023

Thanks @jlenain and @dkerszberg !

@jlenain jlenain deleted the improve-setup branch February 1, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module is not pip-installable in edit mode any more
4 participants