Remove pyplot strong dependency #38

PaulDebus · 2023-07-18T12:29:16Z

As discussed in #37 we could also move the plotting into an extension and make PyPlot a weak dependency. This PR tries this instead of commenting out all mentions of PyPlot.

It should be working well enough, two minor things remain:

changes the default for nonmaxsuppts to not plot the result to work in the default case without PyPlot available
ransacdemo.jl is still not ported, maybe I can get to it at some point

@nlw0 what do you think about this?

Cheers Paul

codecov-commenter · 2023-07-18T12:32:18Z

Codecov Report

Attention: 131 lines in your changes are missing coverage. Please review.

Comparison is base (d753fb8) 46.23% compared to head (5084e4c) 45.59%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
ext/ImageProjectiveGeometryPyPlotExt.jl	0.00%	71 Missing ⚠️
src/plotting.jl	0.00%	32 Missing ⚠️
ext/ransacdemo.jl	0.00%	15 Missing ⚠️
src/utilities.jl	69.76%	13 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   46.23%   45.59%   -0.64%     
==========================================
  Files           8       10       +2     
  Lines        2109     2441     +332     
==========================================
+ Hits          975     1113     +138     
- Misses       1134     1328     +194

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PaulDebus · 2023-07-18T12:37:32Z

Julia LTS (1.4) fails because it does not know about extensions. I naively thought it would be kind of backward compatible, but it apparently is not... I will look into this

peterkovesi · 2023-07-19T13:00:41Z

I would suggest changing CI to only test Julia 1.6 and later as 1.6 was the last LTS

PaulDebus · 2023-07-26T09:57:37Z

The packaging documentation shows a way to move code into an extension while keeping backwards compatibility:

Add the PyPlot dependency back into [deps], which will be ignored in julia 1.9+
Add the following snippet to the main file

if !isdefined(Base, :get_extension)
  include("../ext/ImageProjectiveGeometryPyPlotExt.jl")
end

While this should work, we keep the strong dependency for older Julia<1.9. This keeps the CI complicated and requires projects that want slim dependencies to use at least Julia 1.9+.

The alternative would be to only support new Julia versions, but given that 1.6 is the LTS, this is probably not a good idea...

nlw0 · 2023-07-26T10:11:15Z

Sorry I couldn't come back to this yet. I'm not familiar yet with the weakdep mechanism, but seems promising. I'd say let's just focus on 1.9 moving forward, better to make this a great package depending on modern Julia than to worry about older versions. I'm myself still requiring 1.8 for something, but depending on 1.9 for future versions seems fine to me.

Now that's assuming this is a small fix. Ideally I'd say we really make this a very "mathematical" package, move anything with plotting somewhere else (maybe keep on the same repo, although I'd guess it's easier for julia packages to keep it simple). Now there's just no plotting anywhere, and we retain support for older versions.

nlw0 · 2023-07-26T14:06:01Z

src/ImageProjectiveGeometry.jl

 include("geometry.jl")
+include("plotting.jl")


Shouldn't this include go now inside the extension module?

Could be. I was thinking about allowing different plotting backends, as PyPlot can be quite tricky to set up, and I have used Makie recently, which worked quite well.
I guess my idea was to have some logic that defines the interface of the plotting (which functions need to be implemented) and decides which plotting backend to use - and also is more explicit if somebody tries to use one of the functions.

Looking at the docs again, I think we could also move it into the extension module.

I guess you do not care too much about plotting, so I could look into it and make sure, that all plotting backends have the correct interface.

nlw0 · 2023-07-26T14:06:48Z

src/plotting.jl

+function plotcamera(Ci, l; col=[0,0,1], plotCamPath=false, fig=nothing)
+    ext = get_plot_backend()
+    return ext.plotcamera(Ci, l, col, plotCamPath, fig)
+end


Sorry for nitpicking, please configure your editor to add newlines at the end of files.

Good point, not nitpicky at all.

nlw0 · 2023-07-26T14:11:51Z

src/ImageProjectiveGeometry.jl

@@ -164,8 +164,8 @@ include("transforms3d.jl")
 include("ransac.jl")
 include("cornerfeatures.jl")
 include("utilities.jl")
-include("ransacdemo.jl")
+# include("ransacdemo.jl")


Maybe this could be simply included in the extension module as well (see next comment)

yeah, maybe. It should go somewhere. I guess, we should add the move to the readme, as the demo would be only available if you take additional steps.

nlw0 · 2023-07-26T14:15:15Z

I'd like to try the package out first, but I'm thinking maybe we go ahead with this. Let me get this straight, this keeps compatibility with 1.6, even if CI breaks or whatever, and any 1.6 users might figure out what to do, correct? We merely gain a "new feature" on 1.9, and are able to ignore PyPlot in CI.

I'm thinking this would look like and API change, and perhaps we should now bump a major version? Maybe not now, I guess we can finish the PR first and see if we want to make any other big changes.

PaulDebus · 2023-07-26T15:13:19Z

So this would make a shiny feature in 1.9+, where we have the state we want: plotting works if you manually add PyPlot. If you do not add PyPlot, then no plotting is happening at all and CI for example would be easier (if we do not want to test the plotting code)

For everybody <1.9, a small change (see #38 (comment)) is required, that adds PyPlot as a normal dependency (again). Then, nothing is different from now.

Moving the plotting out of the default functionality would warrant a version change I guess.

So things I want to change before merging:

Make adjustments for Julia <1.9 to work again (see Remove pyplot strong dependency #38 (comment))
Move code from plotting.jl into the extension
move the ransac demo into the extension
update Readme to show the changes and give an explanation on how to get plotting back

This will require CI to still set up Pyplot for the LTS Julia (1.6), which is still broken (see the other discussion).

I hope I did not forget anything... Would you be ok with all that?

- restore compatibility with Julia <1.9 by re-introducing the PyPlot dependency (will be ignored on newer Julia versions) and hard including the extension of weakdeps are not available - move ransacdemo.jl into the extension module - add to README.md how to get plotting back on new Julia - remove the plotting.jl stub, where the functions that extensions need to implement were declared.

PaulDebus · 2023-11-20T21:48:36Z

Hey everyone,

I put some more time into making this work and implemented the changes described above. This does not fundamentally change things, as tests are still broken for older Julia versions. But for Julia >=1.9, we do not need PyPlot anymore.

As a next step, we could use Requires.jl to provide the same functionality, as is also mentioned in the Julia packaging docs. I initially did not like this solution, as it adds more dependencies, but the longer I think about it, the more I like it. The alternative would still be dropping support for Julia <1.9, but I do not want to decide this alone.

What do you say?

Best, Paul

Based on https://pkgdocs.julialang.org/v1/creating-packages/#Backwards-compatibility this uses Requires.jl to bring the weak dependency feature to Julia <1.9

move plotting.jl back in... extensions cannot add new functions, so we already need to define them in a stub that raises an error. We should think about what to export here.

When using Requires and/or package extensions, the PyPlot dependency is not needed anymore. I guess I just forgot to take it out..

peterkovesi · 2023-11-23T08:27:53Z

I personally see little problem with requiring Julia >= 1.9 especially if it makes package development simpler.
I have no idea how many users work with older versions of Julia but I have never encountered any problem when upgrading a Julia version, and upgrading is a simple process.

By requiring a higher Julia version, the package extension mechanism will be available and can be used always (no need for Requires.jl anymore). This makes the implementation easier. The `get_plot_backend()` function is still contained to avoid method overwrite warnings. Usually, a package extension does type piracy and dispatches on a new type. We only have the demo functions, which cannot dispatch on the type. This now can be extended to different plotting backends (Makie for example), which in turn could be selected in the `get_plot_backend()` function.

PaulDebus · 2023-12-27T18:56:27Z

Ok, I also see no problem in requiring a newer Julia version. If this is fine for everybody, then the latest changes should be fine:

require Julia >=1.9
change CI tests to use Julia 1.9 and latest
drop Requires.jl
Bump package version to 0.4.0
Keep the not so nice get_plot_backend function to select the correct backend and route the function calls to the correct package extension. This avoids some method overwrite warnings while loading the extension.

The tests seem to be fine except for the coverage. The plotting functions are not tested, so coverage is not increased and the coverage of the diff is small.

I would appreciate it if somebody could take a look at the resulting changes, to check if I missed something.

Thanks and all the best
Paul

PaulDebus · 2024-02-08T20:24:20Z

Any comments on that? I would like to merge that to get it out of my head (and also simplify the testing for another package I am working on).

If there are no objections, I would merge this soon.

Best, Paul

peterkovesi

Sorry to have held you up. I think it looks good to go. (Sorry about all my trailing spaces on so many lines of code!)

PaulDebus · 2024-02-09T14:19:24Z

No worries, I was also busy and did not keep track.
Thanks for merging, should this trigger a new GitHub release?

peterkovesi · 2024-02-10T03:02:27Z

Yes I guess this should be a new release as a fair bit has changed. Perhaps also add a brief note to the README explaining the plotting changes and the need for Julia 1.9 or greater.

PaulDebus · 2024-02-19T19:37:31Z

I had already added some information to the README in the Installation section. Do you think this is sufficient?

I can of course extend it a bit.

peterkovesi · 2024-02-20T01:23:53Z

What you have added to the README is fine. Sorry, I should have looked first!

Nicolau Leal Werneck and others added 2 commits March 29, 2023 09:41

removing pyplot dependency

61eef92

move plotting into extension and make PyPlot a weak dependency

5abae59

PaulDebus added the enhancement label Jul 18, 2023

PaulDebus requested a review from nlw0 July 18, 2023 12:29

PaulDebus self-assigned this Jul 18, 2023

PaulDebus mentioned this pull request Jul 26, 2023

Update Julia version Compat #39

Open

nlw0 reviewed Jul 26, 2023

View reviewed changes

PaulDebus mentioned this pull request Oct 30, 2023

Reduce dependencies PaulDebus/BundlerIO.jl#31

Open

PaulDebus added 2 commits November 20, 2023 22:25

clarify bad formulation in README

1620d15

PaulDebus added 3 commits November 20, 2023 23:27

use Requires.jl to make weak dependencies work

fb36d68

Based on https://pkgdocs.julialang.org/v1/creating-packages/#Backwards-compatibility this uses Requires.jl to bring the weak dependency feature to Julia <1.9

Another try at fixing this

10a1487

move plotting.jl back in... extensions cannot add new functions, so we already need to define them in a stub that raises an error. We should think about what to export here.

really remove PyPlot dependency

76808d7

When using Requires and/or package extensions, the PyPlot dependency is not needed anymore. I guess I just forgot to take it out..

peterkovesi approved these changes Feb 9, 2024

View reviewed changes

peterkovesi merged commit c9f9f37 into master Feb 9, 2024
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pyplot strong dependency #38

Remove pyplot strong dependency #38

PaulDebus commented Jul 18, 2023

codecov-commenter commented Jul 18, 2023 •

edited

PaulDebus commented Jul 18, 2023

peterkovesi commented Jul 19, 2023

PaulDebus commented Jul 26, 2023 •

edited

nlw0 commented Jul 26, 2023

nlw0 Jul 26, 2023

PaulDebus Jul 26, 2023

nlw0 Jul 26, 2023

PaulDebus Jul 26, 2023

nlw0 Jul 26, 2023

PaulDebus Jul 26, 2023

nlw0 commented Jul 26, 2023

PaulDebus commented Jul 26, 2023 •

edited

PaulDebus commented Nov 20, 2023

peterkovesi commented Nov 23, 2023

PaulDebus commented Dec 27, 2023 •

edited

PaulDebus commented Feb 8, 2024

peterkovesi left a comment

PaulDebus commented Feb 9, 2024

peterkovesi commented Feb 10, 2024

PaulDebus commented Feb 19, 2024

peterkovesi commented Feb 20, 2024

		include("geometry.jl")
		include("plotting.jl")

Remove pyplot strong dependency #38

Remove pyplot strong dependency #38

Conversation

PaulDebus commented Jul 18, 2023

codecov-commenter commented Jul 18, 2023 • edited

Codecov Report

PaulDebus commented Jul 18, 2023

peterkovesi commented Jul 19, 2023

PaulDebus commented Jul 26, 2023 • edited

nlw0 commented Jul 26, 2023

nlw0 Jul 26, 2023

Choose a reason for hiding this comment

PaulDebus Jul 26, 2023

Choose a reason for hiding this comment

nlw0 Jul 26, 2023

Choose a reason for hiding this comment

PaulDebus Jul 26, 2023

Choose a reason for hiding this comment

nlw0 Jul 26, 2023

Choose a reason for hiding this comment

PaulDebus Jul 26, 2023

Choose a reason for hiding this comment

nlw0 commented Jul 26, 2023

PaulDebus commented Jul 26, 2023 • edited

PaulDebus commented Nov 20, 2023

peterkovesi commented Nov 23, 2023

PaulDebus commented Dec 27, 2023 • edited

PaulDebus commented Feb 8, 2024

peterkovesi left a comment

Choose a reason for hiding this comment

PaulDebus commented Feb 9, 2024

peterkovesi commented Feb 10, 2024

PaulDebus commented Feb 19, 2024

peterkovesi commented Feb 20, 2024

codecov-commenter commented Jul 18, 2023 •

edited

PaulDebus commented Jul 26, 2023 •

edited

PaulDebus commented Jul 26, 2023 •

edited

PaulDebus commented Dec 27, 2023 •

edited