Miscelaneous bugfixes (matplotlib & futuretests)

[scarlehoff]

This PR fixes #1808

and on doing this I realized there were some problems with the latest versions of matplotlib.

1. We were using ax._get_lines.prop_cycler which is undocumented and removed in 3.7. I've changed it to the a bit-less-undocumented color = ax._get_lines.get_next_color()
2. The transformation to avoid having points on top of each other doesn't work in matplotlib 3.8, hence the pinning of the version in the conda recipe

   nnpdf/validphys2/src/validphys/plotutils.py

   Line 289 in f553482

   def offset_xcentered(n, ax, *, offset_prop=0.05):

[RoyStegeman]

The transformation to avoid having points on top of each other doesn't work in matplotlib 3.8, hence the pinning of the version in the conda recipe

Could you point me to where this transformation is? Is this something that will "solve itself" in later versions or does this mean we'll be stuck with matplotlib 3.8 unless we do something ourselves?

[scarlehoff]

Is this one here

nnpdf/validphys2/src/validphys/plotutils.py

Line 289 in f553482

def offset_xcentered(n, ax, *, offset_prop=0.05):

we'll be stuck with matplotlib 3.8 unless we do something ourselves

I don't know whether it is a bug in matplotlib 3.8 that will be fixed or a problem that we should fix. I've done a bit of debugging but could not fully understand what the problem is so for the time being I rather have the <

[Radonirinaunimi]

Thanks a lot, this indeed fixes the issue! I just have very minor points below. Apart from the <3.8 part which I might need to take a closer look, this LGTM.

[comane]

Suggested change

	def check_pdf_is_montecarlo_or_symmhessian(pdf, **kwargs):
	etype = pdf.error_type
	@make_check
	def check_pdf_is_montecarlo_or_symmhessian(ns, **kwargs):
	pdf = ns['pdf']
	etype = pdf.error_type

[scarlehoff]

In reportengine it seems make_argcheck is preferred https://github.com/NNPDF/reportengine/blob/79eec2e33c5aab3998fd58e9c3cf84bd2c2696e7/src/reportengine/checks.py#L10

why should we use make_check and ns instead? It also a bit clearer with arcgcheck and pdf since that check is only active for PDFs in any case.

[comane]

Sure, I am ok with that.

[scarlehoff]

But what's the reason for using one over the other?

[comane]

Not sure tbh, I was suggesting it because the other functions like check_pdf_is_montecarlo make use of it and hence it would be more uniform, but I changed my mind.

I think you are right though that make_check should not be used anymore since it is weird that one can access the namespace ns from validphys.

I am thinking that we could change the others to be make_argcheck as well at this point.

[comane]

Suggested change

@make_argcheck

[RoyStegeman]

Wait, this doesn't look quite right

[Radonirinaunimi]

2. The transformation to avoid having points on top of each other doesn't work in matplotlib 3.8, hence the pinning of the version in the conda recipe 

As for this, the issue is more subtle given that transform.ScaledTranslation is expected to have the same behavior as in previous versions, and indeed if one runs the example in v3.8.0 one gets the expected results. So I am fine with excluding >=3.8.0 for the time being.

[scarlehoff]

I tested a bit and the problem is a bit complicated. Basically, while the transformation works ok in some cases, in others something fails internally in matplotlib and the axis are not updated, so the plot looks like this:

by "some cases" I mean some subsets of data.

The same plot, with 3.7 (instead of 3.8) looks like this:

(which is correct) I attempted to fix it but couldn't find what was the reason for the failure so for the time being I rather leave it at <3.8. Indeed, looking at the changelogs of matplotlib I could not find anything that would be broken.

Wait, this doesn't look quite right

I'm using theory 399 in the notebook, which is a low precision theory. If you change it to 400 for instance or 200 it'd be better. But at least now the example runs. We can put something that runs and also looks ok, of course.

[Radonirinaunimi]

I tested a bit and the problem is a bit complicated. Basically, while the transformation works ok in some cases, in others something fails internally in matplotlib and the axis are not updated, so the plot looks like this:
...
(which is correct) I attempted to fix it but couldn't find what was the reason for the failure so for the time being I rather leave it at <3.8. Indeed, looking at the changelogs of matplotlib I could not find anything that would be broken.

These are exactly what I saw when attempting to fix this behavior; but likewise I couldn't really understand what was going on. So fully agreed on leaving <3.8.