insarlab / PySolid

A Python wrapper for solid Earth tides
GNU General Public License v3.0
61 stars 7 forks source link

Cut scikit-image dependency #67

Closed scottstanie closed 10 months ago

scottstanie commented 10 months ago

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 commented 10 months ago

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 ?

yunjunz commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

yunjunz commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Oh whoops! Sorry about cancelling then