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

2024.2 #34

Merged
merged 13 commits into from
Aug 22, 2024
Merged

2024.2 #34

merged 13 commits into from
Aug 22, 2024

Conversation

Chrillebon
Copy link
Collaborator

@Chrillebon Chrillebon commented Aug 6, 2024

This is a draft PR.

(Planned) Changes are listed here:

  • Update matplotlib to new version (currently testing 3.9).
  • Limit numpy to less than 2.0.
  • Update supported Python versions to 3.9-3.12.
  • Update Mayavi version to 4.8.2, allowing for (potential) use of this backend. Update: Too many problems occurred, so I removed the mayavi support. Known problems before we can integrate this: 1. No binary wheel installation through pip. 2. Additional installs required for ubuntu (popouts etc), making the install (and workflow tests) more difficult.
  • Update CI to use "--only-binary", such that workflow only looks for wheels.
  • Add additional tests that check for the "use_cm" flag (which can fail for some spb+matplotlib version interactions).
  • Update to new major spb version (version 3.x.x). This will also fix current issue where the use_cm=True kwarg fails in spb==2.4.3.
  • Update 2D/3D vector plotting to use (new) major spb version functionality
  • (Optional) Change tests, such that different backends are not in the same test (and non-used backends are not tested by default).
  • Fix "(core dumped) pytest" error on workflow for Ubuntu.
  • Add periodic workflow such that we catch potential errors before they reach students.
  • Flexibility in terms of labels. Labels can now be given as both kwargs and in the rendering_kw dictionary. Additionally, latex code should automatically be detected (if written as "$xyz_{abc}$") and written simply as a string (eg. "xyz_{abc}").

When testing for the success of this update, few errors/mistakes etc have been found in the demos. Also, the changes caused by this update can affect some demos. The known things that need to be changed in the demos after this update (before the semester) are:

  • E11: lamb = symbols('\lambda') causes SyntaxWarning: invalid escape sequence '\l'
  • E11: K(3).nullspace() causes TypeError: 'MutableDenseMatrix' object is not callable
  • (Suggestion) F_intro & F1: Few instances of plot(...) (directly calling function from sympy) instead of dtuplot.plot(...). I suggest staying consistent and staying in dtuplot.
  • F1: Instructions to download this package as pip install dtumathtools. This should be updated to reflect the backend and version (for instance pip install dtumathtools[qt]==2024.2)
  • F2: Currently p.xlim and p.ylim are given. This is discussed here and the previous solution is given here. However, the new quiver updates has solved this problem. Therefore, we do not need the limits at all here!
  • F6 and F8: Some of the dtuplot.plot_implicit(...) functions give a warning. Can be worked around by setting the kwarg adaptive=True.
  • F7: We are asking the students to install numpy (which is already done in dtumathtools) and scipy! Should we add scipy as a requirement to avoid having this as part of a demo?

@zerothi
Copy link
Collaborator

zerothi commented Aug 7, 2024

Sadly, pip does not allow one to do combinations or exclusions on --only-binary.
So I think the only way to bypass the maya problem, is to install it in a separate after installing the other things.

@Chrillebon
Copy link
Collaborator Author

Do you want to do a second pip install, or simply remove mayavi from the tests?

Installing it was also not the only problem I had: getting it to work on Ubuntu required a list of additional packages through apt-get to work...

I propose we have the option for people to download it, but do not do anything to test it.

@zerothi
Copy link
Collaborator

zerothi commented Aug 7, 2024

Do you want to do a second pip install, or simply remove mayavi from the tests?

Installing it was also not the only problem I had: getting it to work on Ubuntu required a list of additional packages through apt-get to work...

I propose we have the option for people to download it, but do not do anything to test it.

If it ain't tested (nor used) I think it should be completely removed. We shouldn't propose to use something that is untested, for what-ever reason.

@Chrillebon
Copy link
Collaborator Author

I have updated spb to >=3.1.1, <3.3. I limited to below 3.3, as this removes support for Python 3.9. However, in my testing, I found no problems with newer versions in Python 3.9.
Advantages of spb>=3.4 include support for sympy>=1.13 and numpy>=2.0, so we want this later (maybe when removing support for Python 3.9?).
Let me know if we should handle this differently!

@zerothi
Copy link
Collaborator

zerothi commented Aug 8, 2024

I have updated spb to >=3.1.1, <3.3. I limited to below 3.3, as this removes support for Python 3.9. However, in my testing, I found no problems with newer versions in Python 3.9. Advantages of spb>=3.4 include support for sympy>=1.13 and numpy>=2.0, so we want this later (maybe when removing support for Python 3.9?). Let me know if we should handle this differently!

Otherwise you might want to do conditional dependencies on the python_version? In this way you can allow 3.4 if it is still functional?

@Chrillebon
Copy link
Collaborator Author

I think that is the best option in this case.
Having two different package versions can be a bit of a mess. However, in this case, I think the clear majority will benefit from the bugfixes etc. of the newer versions! Also when we both want it in the future and I have found little to no difference, this is nice to have.

@Chrillebon
Copy link
Collaborator Author

I have found a minor issue when changing the quiver to the spb equivalent. As it might affect some demos, I wanted to give a heads-up.

Automatic limits for quiver has been a problem before. Changing to spb quiver (vector) will (as far as I can see) solve this problem completely in 2D. However, we will run into problems with 3D quivers because of limits with 3Dpatches in matplotlib (see this and this). The effect in spb is that if no limits are given, it will default to a 3x[0,1] interval (which might not include the vector). I propose a solution similar to this, where a qlim argument (maybe as standard) can be given to set the limits of the plot to the edges of the arrow(s). However, this will overwrite any auto limits given by other 3D functions, and as such we need to option to have qlim=False (probably as a default). This is the same behavior as we currently have in both 2D and 3D. In terms of demos, I can highlight the 3D plots in which this might be a problem/where we might need to set the limits manually. I do not imagine this to be a big problem, and plan to continue down this path.

PS. I am still unaware of how this will affect the plotly backend (or any of the other backends), but plotly has behaved significantly better than matplotlib in the past, so I do not think it will be a problem.

@zerothi
Copy link
Collaborator

zerothi commented Aug 9, 2024

I think that is the best option in this case. Having two different package versions can be a bit of a mess. However, in this case, I think the clear majority will benefit from the bugfixes etc. of the newer versions! Also when we both want it in the future and I have found little to no difference, this is nice to have.

Ok.

@Chrillebon
Copy link
Collaborator Author

I have not found the 3D quiver in any demos, so there's no need for extra work here. As this does not allow us to show the use of qlim=True, I have included a warning if the quiver command is used for 3D plotting in matplotlib and neither qlim nor limits are set.

@Chrillebon
Copy link
Collaborator Author

I have gotten through what I wanted for this PR, so I will finish it (make it no longer "a draft").

I have listed the possible changes to the demos above. Most of the affected demos are in the spring. However, we still need to consider if we want to install scipy, to not have this done in a demo later (see the last bullet point).

@Chrillebon Chrillebon marked this pull request as ready for review August 9, 2024 14:31
Copy link
Collaborator

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

Matplotlib has before been a problem, but if your plots works with a broader range of matplotlib versions, that would be nice. :)

"sympy~=1.12",
"matplotlib==3.6.*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it work with older matplotlibs?

@Chrillebon
Copy link
Collaborator Author

Other versions of matplotlib might work. However, as found here, different versions of matplotlib can only be installed on specific versions of python. Matplotlib 3.9.* was chosen as it supports python 3.9-3.12 (the range where we want dtumathtools, and possibly 3.13, when it comes out). Therefore, we can guarantee that all students are running the same matplotlib regardless of python version. As this is also the case for matplotlib 3.8.* (with python 3.13 support being a question mark), we could extend the range to this (eg. matplotlib>=3.8,<3.10). What worries me in extending the range is that some functions and kwargs might change (as far as I am aware) between the versions. Therefore we might have things that work in the demos that do not work for the individual student. As I have not had time to test all the different versions, I cannot say how large this problem is.
If you are confident that a (slightly/much) broader range works and if we are fine with different python versions running different matplotlibs, feel free to update it/add it to the PR (unless anyone else have any opinions?).

@zerothi
Copy link
Collaborator

zerothi commented Aug 12, 2024

When testing for the success of this update, few errors/mistakes etc have been found in the demos. Also, the changes caused by this update can affect some demos. The known things that need to be changed in the demos after this update (before the semester) are:

* [ ]  E11: `lamb = symbols('\lambda')` causes _SyntaxWarning: invalid escape sequence '\l'_

prefix with r'...'

...

* [ ]  F7: We are asking the students to install `numpy` (which is already done in dtumathtools) and `scipy`! Should we add `scipy` as a requirement to avoid having this as part of a demo?

It would be very nice if we (pythonsupport) could install everything you need beforehand...

@zerothi
Copy link
Collaborator

zerothi commented Aug 12, 2024

Other versions of matplotlib might work. However, as found here, different versions of matplotlib can only be installed on specific versions of python. Matplotlib 3.9.* was chosen as it supports python 3.9-3.12 (the range where we want dtumathtools, and possibly 3.13, when it comes out). Therefore, we can guarantee that all students are running the same matplotlib regardless of python version. As this is also the case for matplotlib 3.8.* (with python 3.13 support being a question mark), we could extend the range to this (eg. matplotlib>=3.8,<3.10). What worries me in extending the range is that some functions and kwargs might change (as far as I am aware) between the versions. Therefore we might have things that work in the demos that do not work for the individual student. As I have not had time to test all the different versions, I cannot say how large this problem is. If you are confident that a (slightly/much) broader range works and if we are fine with different python versions running different matplotlibs, feel free to update it/add it to the PR (unless anyone else have any opinions?).
Only newer releases of Matplotlib will support Python 3.13 (no prior releases will be supported by 3.13, see here).

I guess the matplotlib range is more depending on whether your test suite covers the used arguments in the exercises? And if they work? If so I think you should extend the range a bit (as you suggested), otherwise lets keep it.

@Chrillebon
Copy link
Collaborator Author

I have updated the matplotlib version range. Unless we have further comments, I think we could merge now.

@zerothi zerothi merged commit 7a438bd into main Aug 22, 2024
85 checks passed
@zerothi zerothi deleted the 2024.2 branch August 22, 2024 09:11
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.

2 participants