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

Add skimage requirement for CIL-Demos #1955

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Add skimage requirement for CIL-Demos #1955

merged 4 commits into from
Oct 21, 2024

Conversation

lauramurgatroyd
Copy link
Member

@lauramurgatroyd lauramurgatroyd commented Oct 17, 2024

Description

Add requirement of skimage to CIL-Demos install instructions

Required for the new callbacks notebook

Example Usage

Changes

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

Related issues/links

Closes #1956

Checklist

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have updated the relevant documentation
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
  • I confirm that the contribution does not violate any intellectual property rights of third parties

--->

@lauramurgatroyd lauramurgatroyd marked this pull request as ready for review October 17, 2024 08:10
Signed-off-by: Laura Murgatroyd <[email protected]>
Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

This is exceptionally bad practice to put requirements in CIL that have nothing to do with CIL (they are not dependencies).

Please put things in CIL-Demos instead.

At most you could put them in CIL's requirements-test (but not readme etc.) clearly commenting that they're there due to CIL-Demos /cc @paskino

@MargaretDuff
Copy link
Member

I think the point was that the docker image will need to have tomopy and scikit-image installed. It uses requirements-test.yml. It has scikit-image but not tomopy.

@casperdcl
Copy link
Member

casperdcl commented Oct 17, 2024

Exactly. The ONLY correct way to do this is:

  1. drop this entire PR
  2. open another PR to remove all the other cruft from README & requirements-test.yml
  3. open a PR on CIL-Demos & CIL-User-Showcase adding root environment.yml
  4. open a PR on CIL to make the Dockerfile use conda install -n base --file=https://raw.githubusercontent.com/TomographicImaging/CIL-{Demos,User-Showcase}/refs/heads/main/environment.yml

@lauramurgatroyd
Copy link
Member Author

Tomopy is no longer required so updating this PR to just add skimage to the CIL-Demos install instructions and I will open another issue with Casper's suggetions

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/requirements-test.yml Outdated Show resolved Hide resolved
Signed-off-by: Laura Murgatroyd <[email protected]>
@lauramurgatroyd lauramurgatroyd changed the title Add tomopy and skimage requirement for CIL-Demos Add skimage requirement for CIL-Demos Oct 18, 2024
@lauramurgatroyd
Copy link
Member Author

@casperdcl thanks for your suggestions, hopefully this covers everything: #1961

We need a wider discussion / agreement before removing the CIL-Demos install instructions from the CIL readme, so I still need to add skimage to the install instructions in this PR

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@hrobarts hrobarts left a comment

Choose a reason for hiding this comment

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

Looks good

@MargaretDuff MargaretDuff dismissed casperdcl’s stale review October 21, 2024 12:37

Have implemented Casper's suggestions

@MargaretDuff MargaretDuff merged commit 662bad3 into master Oct 21, 2024
1 check passed
@MargaretDuff MargaretDuff deleted the add_tomopy_req branch October 21, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scikit-image needed for new CIL-Demos
4 participants