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

Cut scikit-image dependency #67

Merged
merged 10 commits into from
Aug 16, 2023
Merged

Conversation

scottstanie
Copy link
Collaborator

@scottstanie scottstanie commented Aug 14, 2023

The resize function is a thin wrapper around ndimage.zoom. There were only a few names changed.

The add an anti-aliasing filter, which might make more of a difference in other cases, but the SET is so smooth that it doesn't matter for us. The difference is approximately 0:

image

@scottstanie
Copy link
Collaborator Author

I was also thinking of removing matplotlib-base as a dependency, since it's really just used to inspect the outputs. What do you think of that @yunjunz ?

src/pysolid/grid.py Outdated Show resolved Hide resolved
src/pysolid/grid.py Outdated Show resolved Hide resolved
@yunjunz
Copy link
Member

yunjunz commented Aug 15, 2023

I was also thinking of removing matplotlib-base as a dependency, since it's really just used to inspect the outputs. What do you think of that @yunjunz ?

matplotlib-base is definitely optional and could have been removed from the conda-forge recipe at least. I am hesitant to remove it from the requirements.txt because people may find it confusing as they thought they have install the code but could not run the notebook or example code on the readme. matplotlib is so widely used, is there a specific reason we want to not installing it?

@scottstanie
Copy link
Collaborator Author

matplotlib is so widely used, is there a specific reason we want to not installing it?

The specific reason was to try and make downstream packages that use PySolid (like COMPASS) smaller.

If I take the environment.yml file in this PR and make a lockfile with all the dependencies using this script, the total number of conda packages goes from 67 to 173

$ ../dolphin/docker/create-lockfile.sh environment.yml > pysolid_with_matplotlib.txt
# Then remove matplotlib-base in `environment-no-mpl.yml`
$  ../dolphin/docker/create-lockfile.sh environment-no-mpl.yml > pysolid_no_matplotlib.txt

$ wc -l pysolid_*_matplotlib.txt
   67 pysolid_no_matplotlib.txt
  173 pysolid_with_matplotlib.txt

And building when docker image with those two environments fies, the one with matplotlib is about double in size the one without:

REPOSITORY          TAG               IMAGE ID       CREATED          SIZE
insarlab/pysolid    with-matplotlib   d90399eaefdb   25 seconds ago   1.85GB
insarlab/pysolid    no-matplotlib     a8ae2dc6fbd3   57 seconds ago   934MB

So in practice you're right that for a lot of people who are going to install pysolid into some big all-encompasing environment, it makes no difference. but if (and when) isce3 splits off from NISAR and cuts down it's dependencies, it'll be nice for environemnts to solve faster and docker images to be smaller.
We can push off that change to another PR that also would update documentation if you'd like to keep this one focused

@hfattahi
Copy link

matplotlib is so widely used, is there a specific reason we want to not installing it?

The specific reason was to try and make downstream packages that use PySolid (like COMPASS) smaller.

If I take the environment.yml file in this PR and make a lockfile with all the dependencies using this script, the total number of conda packages goes from 67 to 173

$ ../dolphin/docker/create-lockfile.sh environment.yml > pysolid_with_matplotlib.txt
# Then remove matplotlib-base in `environment-no-mpl.yml`
$  ../dolphin/docker/create-lockfile.sh environment-no-mpl.yml > pysolid_no_matplotlib.txt

$ wc -l pysolid_*_matplotlib.txt
   67 pysolid_no_matplotlib.txt
  173 pysolid_with_matplotlib.txt

And building when docker image with those two environments fies, the one with matplotlib is about double in size the one without:

REPOSITORY          TAG               IMAGE ID       CREATED          SIZE
insarlab/pysolid    with-matplotlib   d90399eaefdb   25 seconds ago   1.85GB
insarlab/pysolid    no-matplotlib     a8ae2dc6fbd3   57 seconds ago   934MB

So in practice you're right that for a lot of people who are going to install pysolid into some big all-encompasing environment, it makes no difference. but if (and when) isce3 splits off from NISAR and cuts down it's dependencies, it'll be nice for environemnts to solve faster and docker images to be smaller. We can push off that change to another PR that also would update documentation if you'd like to keep this one focused

We definitely need to remove matplotlib for obvious reasons that @scottstanie mentioned above. It actually came up already on data system side. Up to you guys if you want to do that in this PR or a separate PR.

Copy link
Member

@yunjunz yunjunz left a comment

Choose a reason for hiding this comment

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

Looks all good to me. Thank you @scottstanie!

@yunjunz
Copy link
Member

yunjunz commented Aug 16, 2023

Removing matplotlib in another PR sounds good to me. I like your tests/requirements.txt file in dolphin, so we could add matplotlib there.

Regarding the dependency files, I believe environment.yml file is redundant, we could remove it as well.

@yunjunz
Copy link
Member

yunjunz commented Aug 16, 2023

The CI looked normal to me. It usually takes ~1 hour to finish all the testing on various Linux versions (https://github.com/insarlab/PySolid/actions/runs/5570498743/job/15083276586?pr=63).

@scottstanie
Copy link
Collaborator Author

Oh whoops! Sorry about cancelling then

@yunjunz yunjunz merged commit c27c8d7 into insarlab:main Aug 16, 2023
4 checks passed
@yunjunz yunjunz mentioned this pull request Aug 16, 2023
@scottstanie scottstanie deleted the cut-dependencies branch August 16, 2023 12:09
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.

3 participants