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

Update matplotlib version #2191

Merged
merged 10 commits into from
Nov 22, 2024
Merged

Update matplotlib version #2191

merged 10 commits into from
Nov 22, 2024

Conversation

Radonirinaunimi
Copy link
Member

A simple change to address #2162 and the issue reported in #1809 related to the axis transformation for matplotlib >= 3.8. This also seems to fix the non-deterministic failing tests in #2099 (at least for me locally).

PS: Need to be tested further.

@Radonirinaunimi Radonirinaunimi marked this pull request as draft October 30, 2024 14:41
@scarlehoff
Copy link
Member

Nice! I would be happy with merging it and crossing our fingers since it seems to work also in the ci

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Oct 30, 2024
@Radonirinaunimi
Copy link
Member Author

Nice! I would be happy with merging it and crossing our fingers since it seems to work also in the ci

I still need to perform further tests but indeed it seems to work. After understanding the issues, I think it happened that magically it worked before v3.8.

Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff
Copy link
Member

I'd be happy merging this even if it doesn't fix the random failure. If it does, so much the better.

What @RoyStegeman said about the DW variants worries me because those are relatively recent, if it fails again I'll have a deeper look.

Thanks!

@Radonirinaunimi Radonirinaunimi added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Oct 30, 2024
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@Radonirinaunimi
Copy link
Member Author

I had to re-run the fitbot because the first one was started before the change in the conda-recipe/meta.yaml. So the last reports are the correct ones.

So , yes, this basically solves the issues with the offset plotting with a still minor issue (which should be easy to solve) that for some very few ratio plots there are points that are cut ways: for e.g. reference vs now.

@scarlehoff
Copy link
Member

i see, do you have an idea of how to solve that? Re-axing (if that's a thing?)

@Radonirinaunimi
Copy link
Member Author

i see, do you have an idea of how to solve that? Re-axing (if that's a thing?)

Not yet xd but I will have a look in a while. It shouldn't be difficult.

@RoyStegeman
Copy link
Member

RoyStegeman commented Oct 31, 2024

Something I didn't realise before: I can reproduce this error locally (while yesterday I said I couldn't), but while it occurs if I run all tests it doesn't if I try to run only the failing tests

Are tests always executed in the same order? Perhaps there is a race condition for an @lru_cache or something like that?

@Radonirinaunimi
Copy link
Member Author

So I haven't found a fancy way to construct the ranges so the $y$ limits are computed manually f7c6595.

Something I didn't realise before: I can reproduce this error locally (while yesterday I said I couldn't), but while it occurs if I run all tests it doesn't if I try to run only the failing tests

Just to confirm that locally, sometimes, I am still experiencing this issue. So updating dependency versions definitely does not magically solve it.

@RoyStegeman
Copy link
Member

Do you experience the issue also if you only run the failing tests?

@Radonirinaunimi
Copy link
Member Author

Do you experience the issue also if you only run the failing tests?

I've never experienced it when running separately the failing tests. But also it's not every time that tests are failing when I run everything all at once. It seems to occur randomly when running all the tests altogether...

@RoyStegeman
Copy link
Member

RoyStegeman commented Oct 31, 2024

Yes same here, I tried two more times running all locally but they just passed

@Radonirinaunimi Radonirinaunimi added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Oct 31, 2024
@Radonirinaunimi
Copy link
Member Author

Yes same here, I tried two more times running all locally but they just passed

Hmhmh, this issue deserves a closer look. If we are happy with the last fitbot, we can merge this and investigate the issue in another PR.

@RoyStegeman
Copy link
Member

I think the problem might be the caching of parse_new_metadata. I'll have a closer look tomorrow

@scarlehoff
Copy link
Member

The latest fitbot actually picked up a problem!

RE the failure, I agree, let's investigate it somewhere else.

@Radonirinaunimi
Copy link
Member Author

The latest fitbot actually picked up a problem!

Yes, the error is that for some instances all the values are all NaNs, so it will be sufficient to simply skip these instances when setting ax.get_ylim. I missed this before because I haven't checked all the datasets that are run/fitted by the bot.

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Nov 1, 2024
@Radonirinaunimi Radonirinaunimi added the run-fit-bot Starts fit bot from a PR. label Nov 1, 2024
Copy link

github-actions bot commented Nov 1, 2024

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff marked this pull request as ready for review November 13, 2024 16:05
@scarlehoff
Copy link
Member

scarlehoff commented Nov 22, 2024

There was still a problem with plots with log axes and plots which might have several axis in one figure, this example suffers from both: https://vp.nnpdf.science/ju590ltLStii5BG9mkW-Hw==/matched_datasets_from_dataspecs24_dataset_report_report.html#normalized

I've pushed my solution (+ rebase on top of master). Basically I use the existing ymax/ymin instead of recalculating it, seems to work locally... let's see whether the bot thinks the same thing and we are not just playing whack-a-mole where fixing one plot breaks the following one.

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Nov 22, 2024
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@Radonirinaunimi
Copy link
Member Author

I've pushed my solution (+ rebase on top of master). Basically I use the existing ymax/ymin instead of recalculating it, seems to work locally... let's see whether the bot thinks the same thing and we are not just playing whack-a-mole where fixing one plot breaks the following one.

This is what I first tried before but didn't work locally because (back then if I remember correctly the issue was) the values of the mins/maxs were computed based on the data cv+/-err and therefore might cut away the predictions. But looking at the reports, everything seems to be perfect now. So, good for it.

The only thing remaining here is updating the regression plot.

@scarlehoff
Copy link
Member

I'll try to update the plot later. The change is basically a shift, maybe there's a way of making the test more stable.

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Nov 22, 2024
@scarlehoff
Copy link
Member

Since the python installation is already checking the plot, I put a less restrictive check for the conda installation (I'll increase it until the tests pass).

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

LGTM! If you are happy with my change to the tests please merge ^^

I've checked all plots in the bot's report and I think they are ok, but at some point the points were dancing around my eyes... I hope I haven't missed anything.

@Radonirinaunimi
Copy link
Member Author

I've checked all plots in the bot's report and I think they are ok, but at some point the points were dancing around my eyes... I hope I haven't missed anything.

I've also gone through all of them before, so let's hope that the parts where the points started dancing in our case did not overlap xD

I'm merging now.

@Radonirinaunimi Radonirinaunimi merged commit a329696 into master Nov 22, 2024
6 checks passed
@Radonirinaunimi Radonirinaunimi deleted the update-matplotlib branch November 22, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants