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

Automate regression updates #1944

Merged
merged 6 commits into from
Feb 16, 2024
Merged

Automate regression updates #1944

merged 6 commits into from
Feb 16, 2024

Conversation

APJansen
Copy link
Collaborator

@APJansen APJansen commented Feb 16, 2024

Aim

Be able to add a red-regressions label and have the regressions be recomputed in the CI and automatically committed.

Status

It seems to be working for the extra regression tests, the result of the workflow applied to this PR as a test is the commit d142744

I don't know about the fitbot, if we want to automate that too I guess it would be a different workflow but using the same label. And what would need to be updated exactly? The developing_weights are a starting point, so I assume those stay the same. Would you want to update the reference fit that it compares against?

Also, I expected the bot's push to trigger the other tests again, but it seems this doesn't happen. I don't know if that's necessary or not.

@APJansen APJansen mentioned this pull request Feb 16, 2024
@APJansen APJansen added redo-regressions Recompute the regression data run-fit-bot Starts fit bot from a PR. and removed redo-regressions Recompute the regression data run-fit-bot Starts fit bot from a PR. labels Feb 16, 2024
@APJansen APJansen force-pushed the automate-regression-updates branch from cd2e36f to 004fdf1 Compare February 16, 2024 11:12
@APJansen APJansen added redo-regressions Recompute the regression data and removed redo-regressions Recompute the regression data labels Feb 16, 2024
@APJansen APJansen force-pushed the automate-regression-updates branch from faa9442 to 7c73713 Compare February 16, 2024 11:41
@APJansen APJansen added redo-regressions Recompute the regression data and removed redo-regressions Recompute the regression data labels Feb 16, 2024
@APJansen APJansen marked this pull request as ready for review February 16, 2024 12:10
@APJansen APJansen requested a review from scarlehoff February 16, 2024 12:10
@scarlehoff
Copy link
Member

Thanks!

I think git bot commits cannot trigger again the checks. Let's see whether having the previous check is enough for the branch rules to consider the branch as ok.

Also, I like that we will basically have a redo-regression tag that would allow to quickly know whether they were regenerated in a particular branch!

@APJansen APJansen removed the redo-regressions Recompute the regression data label Feb 16, 2024
@APJansen APJansen added the redo-regressions Recompute the regression data label Feb 16, 2024
@APJansen APJansen added redo-regressions Recompute the regression data and removed redo-regressions Recompute the regression data labels Feb 16, 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 😉!

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 😉!

@APJansen APJansen force-pushed the automate-regression-updates branch from 10c7d1b to 5adb063 Compare February 16, 2024 12:45
@APJansen
Copy link
Collaborator Author

Interesting that the .exportgrid didn't change for any of the fits while the integrability and arc-lenghts did. arc-length changes are due to scipy's integrate module, fine, but the integrability surprises me.

They just changed again, so there is still some source of randomness somewhere. Not really an issue though I think, changes are very small.

@scarlehoff
Copy link
Member

Yes, sure. I'm just surprised that integrability changed while the .exportgrid (which contains hundreds of values) didn't.

The integrability asks the NN for a bunch of points and sums them together, while the .exportgrid asks the NN for hundreds of values...

(in any case I agree, it doesn't really matter. I just found it curious).

@APJansen APJansen added redo-regressions Recompute the regression data and removed redo-regressions Recompute the regression data labels Feb 16, 2024
@APJansen
Copy link
Collaborator Author

Yes, sure. I'm just surprised that integrability changed while the .exportgrid (which contains hundreds of values) didn't.

To be sure everythings ok I checked that if I run it locally, those do get changed.

What about the fit-bot?

@APJansen
Copy link
Collaborator Author

They just changed again, so there is still some source of randomness somewhere. Not really an issue though I think, changes are very small.

Actually that's not true, looking a bit more carefully, only the timings changed now.

@scarlehoff
Copy link
Member

What do you mean?

@APJansen
Copy link
Collaborator Author

Earlier I thought the regression data was changed twice, so there was some randomness still, but I think I wasn't looking carefully enough, it was only the timings that changed the second time.

Or if you mean on the fit bot:

I don't know about the fitbot, if we want to automate that too I guess it would be a different workflow but using the same label. And what would need to be updated exactly? The developing_weights are a starting point, so I assume those stay the same. Would you want to update the reference fit that it compares against?

@scarlehoff
Copy link
Member

Ah, no, don't update the fitbot like this.
That's a more conscious choice (and it's really just changing the reference in the workflow, nothing needs to be rerun)

@APJansen APJansen force-pushed the automate-regression-updates branch from c6e74ec to 8d3ecc0 Compare February 16, 2024 13:30
@APJansen
Copy link
Collaborator Author

Ok, then this is done I think? I just rebased to master.

@RoyStegeman
Copy link
Member

Thanks! This is nice!

@scarlehoff scarlehoff mentioned this pull request Feb 16, 2024
34 tasks
@scarlehoff scarlehoff merged commit 4737e16 into master Feb 16, 2024
8 checks passed
@scarlehoff scarlehoff deleted the automate-regression-updates branch February 16, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redo-regressions Recompute the regression data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants